diff --git a/Docs/architecture.md b/Docs/architecture.md index 7f2a4e23..ee48c9bf 100644 --- a/Docs/architecture.md +++ b/Docs/architecture.md @@ -1,113 +1,104 @@ -# CodexReviewKit Architecture +# Architecture -CodexReviewKit provides ReviewMonitor, a native macOS app for running and -observing Codex review. The package has one observable review store, one Codex -app-server gateway, and an internal MCP adapter owned by the app. - -The package is organized around four ownership boundaries: - -- `CodexReview` owns review behavior and observable product state. -- `CodexReviewAppServer` owns all `codex app-server` JSON-RPC I/O. -- `CodexReviewMCPServer` converts app-managed MCP tool calls to review commands. -- `CodexReviewHost` assembles concrete live dependencies. - -`ReviewUI` renders the monitor state and forwards user intent. It does not own -review rules, app-server protocol details, persistence, or process lifecycle. +ReviewMonitor's UI and MCP server share one `CodexReviewStore`. The store owns +review state and commands; its backend sends requests to `codex app-server`. +`CodexReviewHost` connects these components when the app starts. ## Targets | Target | Responsibility | | --- | --- | -| `CodexReview` | Review API, `CodexReviewStore`, observable state, and product invariants | -| `CodexReviewAppServer` | `codex app-server` JSON-RPC protocol, process transport, request serialization, notifications | -| `CodexReviewMCPServer` | Internal MCP tool request/response conversion and Streamable HTTP endpoint | -| `CodexReviewHost` | Runtime composition for ReviewMonitor | -| `CodexReviewTesting` | Deterministic fake backend, fake JSON-RPC transport, gates, manual clock | -| `ReviewUI` | Native monitor UI rendering and user-intent forwarding | - -ReviewMonitor is the product entry point. Review behavior, Codex protocol -handling, MCP conversion, and UI rendering remain in their owning targets. - -## Runtime Flow - -ReviewMonitor composes `ReviewUI` and `CodexReviewMCPServer` over one -`CodexReviewStore`. Its live backend delegates Codex I/O through -`CodexReviewHost` to `CodexReviewAppServer`; dependencies do not point back -toward the app or UI. - -### Codex Updates - -`CodexReviewStore.updateCodex(when:install:)` owns the update operation, its -observable progress, review dispatch suspension, and runtime replacement. It -retains MCP sessions and accepted jobs, joins existing execution and cleanup, -then resumes dispatch after runtime publication. Repeated update requests join -the same operation. Runtime recovery confirms the owned process has closed -without treating a recorded close error as permanent evidence that it is live. - -The app's `ReviewMonitorCodexUpdater` owns update checks and their results. The -sidebar receives its availability projection, and Settings uses the same -instance. A continuous clock anchors automatic checks to launch and eight-hour -boundaries; manual checks share in-flight work without moving that anchor. -Checks due during installation are covered by one post-update check. - -Application termination stops read-only checking, shuts down the Store, and -joins startup before replying to AppKit. The Store can cancel a deferred update -or wait for an installation already in progress. Codex updates do not use an -application relaunch helper. - -## CodexReview - -`CodexReviewStore` is the single source of truth for review, runtime, auth, -settings, workspace, job, and log state. It is also the command owner for -`review_start`, `review_await`, `review_read`, `review_list`, `review_cancel`, session close, -auth actions, and settings updates. UI and MCP both use the same store API. - -`CodexReviewStoreBackend` is the dependency boundary below the store. Live, -preview, and test backends all implement that boundary; product state remains in -the store. - -## App-Server Gateway - -`CodexReviewAppServer` treats raw JSON-RPC as the only I/O boundary. - -- One live `codex app-server` process maps to one shared connection. -- `initialize` and `initialized` run once per connection. -- App-server operations are typed at the request boundary. -- Same-thread mutating requests are serialized. -- Different-thread requests may run concurrently. -- `turn/interrupt` is a control request and is not queued behind an in-flight - same-thread `turn/start`. -- Reviews run as a normal `turn/start` on the thread created for that review. - One adapter renders the typed review target and references the server-reported - `$review-agent` skill provisioned during app-server initialization; the - request carries the review working directory. -- Notifications are subscribed before `turn/start` so terminal events emitted - with the response are not lost. -- Cancellation is represented by typed control/cleanup requests, not by closing - the transport. +| `CodexReview` | Review API, observable store, authentication, settings, and history contracts | +| `CodexReviewAppServer` | JSON-RPC requests, notifications, and process transport for `codex app-server` | +| `CodexReviewPersistence` | SQLite history storage, migrations, and retention | +| `CodexReviewMCPServer` | MCP tool conversion and the Streamable HTTP endpoint | +| `CodexReviewHost` | Live backend, filesystem locations, and dependency assembly | +| `CodexReviewTesting` | Fake backend and transport, gates, and manual clock | +| `ReviewUI` | Monitor views and controllers | +| `TextTransitions` | Animated text rendering | + +ReviewMonitor is the app entry point. The package exports `CodexReview`, +`CodexReviewHost`, `ReviewUI`, and `TextTransitions` as libraries; the other +production targets support those libraries internally. + +## Review flow + +Both UI actions and MCP tools call the store. `CodexReviewStoreBackend` supplies +runtime operations, with live, preview, and test implementations. Each uses the +same store to manage product state. + +The live backend uses one shared connection to a long-lived `codex app-server` +process. The gateway initializes the connection once with `initialize` and +`initialized`, then sends typed requests: + +- Mutating requests on the same thread run in order. Requests on different + threads can run concurrently. +- A review uses a normal `turn/start` on its review thread. The adapter includes + the target, working directory, and server-reported `$review-agent` skill + provisioned during initialization. +- The gateway subscribes to notifications before `turn/start` so it can receive + terminal events sent alongside the response. +- `turn/interrupt` can proceed while `turn/start` is pending. Cancellation uses + control and cleanup requests; it keeps the transport open. Fake and live tests use the same transport protocol. -## MCP Boundary - -`CodexReviewMCPServer` knows MCP tool names, request arguments, and response -shape. It calls `CodexReviewStore` commands and does not know Codex JSON-RPC -details. - -ReviewMonitor owns the default Streamable HTTP endpoint at -`http://localhost:9417/mcp`. The HTTP boundary follows current MCP session -semantics: `initialize` creates an `MCP-Session-Id`, subsequent requests carry -that session header, responses are delivered as JSON or SSE as negotiated by the -client, and `DELETE` closes a session. Tool and response contracts live in the -[MCP reference](mcp.md). - -## Monitor UI Boundary - -`ReviewUI` observes `CodexReviewStore` directly. - -- Views and view controllers render observable state. -- User actions call store methods. -- UI tests cover layout, selection, rendering, accessibility-facing text, and - user-intent forwarding. -- Review/auth/settings semantics are tested in `CodexReviewTests` and - `CodexReviewAppServerTests`. +## MCP sessions + +The app hosts `http://localhost:9417/mcp`. An `initialize` request creates an +`MCP-Session-Id`; subsequent requests carry that header. The server returns JSON +or SSE according to client negotiation, and `DELETE` closes the session. + +Jobs belong to the session that started them. The MCP adapter converts tool +arguments into store commands and converts their results into MCP responses. +The app-server gateway handles Codex's JSON-RPC separately. See the +[MCP reference](mcp.md) for tool and response fields. + +## History and UI + +The store loads and saves history through `CodexReviewPersistence`, which owns +the SQLite database. `CodexReviewHost` supplies its filesystem location. History +stores review metadata, final results, and findings; live transcripts stay in +memory. Restored reviews appear in the UI but are unavailable to new MCP +sessions. + +`ReviewUI` observes the store and forwards user actions to it. UI tests cover +layout, selection, rendering, accessibility text, and action forwarding. +`CodexReviewTests` and `CodexReviewAppServerTests` cover review, authentication, +settings, and protocol behavior. + +## Codex updates + +`CodexReviewStore.updateCodex(when:install:)` pauses new review execution while +keeping accepted jobs and MCP sessions. It waits for current reviews or cancels +them according to the requested timing, finishes cleanup, stops the old runtime, +runs the installation closure, and starts the replacement runtime. Queued jobs +resume once that runtime is ready. Concurrent update calls wait for the same +operation, using the first call's installation closure. + +Recovery checks whether the owned process has closed. A previously recorded +close error alone does not prevent recovery if the process is now closed. + +The app's `ReviewMonitorCodexUpdater` owns update checks and their results. +Settings and the sidebar share that instance. Automatic checks run at launch +and at eight-hour intervals measured from launch. Manual checks join a check +already in progress and keep that schedule. A check due during installation +runs once after the update. + +On application termination, the app stops checking, shuts down the store, and +waits for startup work before replying to AppKit. Store shutdown cancels an +update still waiting for reviews or waits for an installation already in +progress. Codex updates restart the runtime within the running app. + +### Simulate an update + +To inspect UI responsiveness, set `REVIEW_MONITOR_SIMULATE_CODEX_UPDATE=1` in the +Xcode scheme's **Run → Arguments → Environment Variables** and launch the app. +Use the live runtime with `REVIEW_MONITOR_MOCK_JOBS` and +`REVIEW_MONITOR_REVIEW_MODE` disabled. + +After a one-second simulated check, the normal **Update** button appears. The +installation step waits ten seconds through the same subprocess runner as a +real update. The existing Codex runtime stops and restarts normally, while the +Codex and Homebrew packages stay unchanged. Settings then shows **Up to Date**. +Relaunch the app to repeat the simulation. diff --git a/Docs/deferred-codex-update-design.md b/Docs/deferred-codex-update-design.md index 1b498777..fb44c923 100644 --- a/Docs/deferred-codex-update-design.md +++ b/Docs/deferred-codex-update-design.md @@ -1,16 +1,24 @@ -# レビュー要求を Kit 内で待たせて Codex を更新する +# レビューを待たせて Codex を更新する -状態: 2026-09-26 承認済み。基準は `b19308aa72492797cb4fd7abfbbb28735b4d7cd0`。実装は [親 Issue #382](https://github.com/lynnswap/CodexReviewKit/issues/382) の sub-issue 順に進める。 +2026-09-26 に承認した設計。基準 commit は +`b19308aa72492797cb4fd7abfbbb28735b4d7cd0`、実装の分割は +[親 Issue #382](https://github.com/lynnswap/CodexReviewKit/issues/382) に記録する。 +ここでいう変更前の動作は、この commit 時点のもの。 +現在の構成は [architecture.md](architecture.md#codex-updates) を参照。 -「後でアップデート」を選ぶと、実行中のレビューを最後まで続け、新たな要求を `CodexReviewStore` のキューに受け付ける。実行中のレビューが終わったら Codex を更新し、app-server を再起動してキューを処理する。Monitor と MCP サーバーは動かしたままにする。 +「後でアップデート」を選ぶと、実行中のレビューを最後まで続け、以後の要求を +`CodexReviewStore` のキューに入れる。レビューが終わったら Codex を更新し、 +app-server を再起動してキューのレビューを開始する。Monitor と MCP セッションは +その間も維持する。 -LLM は従来どおり `review_start` を一度呼ぶ。キュー待ちを理由に即座に応答を返したり、要求の再送・待機判断を LLM に任せたりしない。 +## 一度の呼び出しで受付から結果を待つ -## 受付から完了までを同じジョブとして扱う +クライアントは従来どおり `review_start` を一度呼ぶ。Store は要求を検証し、 +ジョブ ID とセッションの所有権を割り当て、履歴を保存する。更新予約中は +`Queued` と表示し、app-server のスレッドや turn の作成を待たせる。 -更新予約後も `review_start` を受け付け、既存のジョブ ID とセッション所有権を割り当てる。要求の妥当性と履歴の保存を確認した後、Monitor では `Queued` と表示する。まだ app-server のスレッドや turn は作成しない。 - -キューに積んだ要求は、実行中レビューの完了、更新、app-server の起動をサーバー内部で待つ。MCP 呼び出しは通常の完了待機につなぎ、同じジョブの最終結果を返す。状態確認とキャンセルは、更新中も通常の MCP API から使える。 +MCP 呼び出しは、キュー待ち、更新、レビュー実行を通して同じジョブの結果を待つ。 +クライアントに要求を再送させない。状態確認とキャンセルは更新中も使える。 ```mermaid sequenceDiagram @@ -31,59 +39,64 @@ sequenceDiagram S-->>L: review_start(B) の結果 ``` -「後で」の選択後に届いた要求をキューに保持するため、新しい要求が続いても更新機会は失われない。すでに起動処理に入っていたレビューも、完了を待つ対象に含める。履歴保存中の新規要求は、保存が終わった時点で現在の更新状態を確認してキューへ入れる。 - -再開時は受付順に既存のレビュー実行処理へ渡す。通常のレビュー並行実行は維持し、1件ずつ完了まで待つ新しい直列実行制約は設けない。受付順には履歴保存の完了順ではなく、Store が要求を受け付けた順序を使う。 +予約後の新規要求はすべてキューに入るため、要求が続いても更新を始められる。 +予約前に起動処理へ進んだレビューは完了を待つ。履歴保存中の要求は、保存後に +更新状態を確認して実行か待機を決める。 -### MCP の既存の待機上限は維持する +更新後は Store の受付順でレビューを開始する。履歴の保存完了順は使わない。 +開始後は通常どおり並行実行し、先のレビューが終わるまで次を止めることはしない。 -現在の `review_start` と `review_await` は最大540秒で応答する。キュー待ちもこの呼び出し全体の待機時間に含める。HTTP の heartbeat は接続維持に使い、LLM 向けの新しいメッセージにはしない。 +### MCP の待機上限は540秒 -上限までに完了しなかった場合だけ、既存形式のジョブ ID と `queued` / `running` を返す。クライアントは従来の `review_await` で同じジョブを待てる。キューから削除したり、再度 `review_start` を要求したりしない。接続切断もキューの再投入には結び付けない。 +`review_start` と `review_await` の上限は、キュー待ちを含めて540秒とする。 +完了しなければ既存形式のジョブ ID と `queued` / `running` を返し、クライアントは +`review_await` で同じジョブを待てる。時間切れや接続切断でキューへ再投入しない。 +HTTP heartbeat は接続維持に使い、LLM 向けのメッセージを増やさない。 -この上限を超えても LLM の追加呼び出しを完全になくす保証は、本変更には含めない。固定タイムアウトを持つクライアント側の契約も変える必要があるためである。 +540秒を超える場合は追加の待機呼び出しが必要になる。これをなくすには、固定の +タイムアウトを持つクライアント側の契約も変更する必要がある。 -## 実装済みの処理から変更する点 +## 受付と実行開始を分ける -| 現在の処理 | 本変更で必要な動作 | +| 変更前の動作 | この設計での変更 | | --- | --- | -| `beginReview` は履歴を保存すると直ちに worker を作り、`running` にする。 | 要求の受付・保存と、worker の開始を分ける。更新予約中はジョブをキューで保持する。 | -| `hasRunningJobs` はすべての未完了ジョブや待機処理を含む。 | 更新開始の判定には、app-server を使用中のレビューとその後片付けを使う。新たにキューへ入ったジョブを完了待ちの対象にしない。 | -| `admitRuntimeReplacement` はレビュー受付を閉じ、履歴保存中の開始要求をキャンセルする。 | 計画的な更新では受付・参照・キャンセルを維持する。実行開始だけをキューで保留する。 | -| app-server 終了時には未完了レビューをまとめてキャンセルする。 | 更新時の終了処理は、実行前のキューをキャンセルしない。明示的なアプリ終了・サインアウトの契約とは区別する。 | -| 更新承認後に `store.shutdown()`、`codex update`、Monitor 全体の再起動を行う。 | Kit が既存 MCP セッションを保って app-server だけを終了・再起動する。 | -| 自動確認は起動時と確認後20時間の間隔で、設定画面には確認操作がない。 | 起動時と起動から8時間ごとに確認し、同じ checker を設定画面からも呼べるようにする。 | +| `beginReview` は保存後すぐに worker を作り、`running` にする | 受付・保存と worker 開始を分け、更新予約中は待たせる | +| `hasRunningJobs` はすべての未完了ジョブと待機処理を含む | 更新は app-server を使用中のレビューと後片付けだけを待つ | +| `admitRuntimeReplacement` は受付を閉じ、保存中の開始要求をキャンセルする | 更新では受付・参照・キャンセルを維持し、実行開始を保留する | +| app-server 終了時に未完了レビューをまとめてキャンセルする | 更新では実行前のキューを保持する。アプリ終了とサインアウトの契約は保つ | +| `store.shutdown()`、更新、Monitor 全体の再起動を行う | Kit が app-server だけを入れ替え、MCP セッションを維持する | +| 起動時と確認後20時間ごとに確認し、設定画面には確認操作がない | 起動時と起動から8時間ごとに確認し、設定画面からも同じ checker を使う | -参照: +関連する実装: - [レビューの受付と worker 開始](../Sources/CodexReview/Store/CodexReviewStoreReviews.swift) - [Store の runtime 差し替えと未完了判定](../Sources/CodexReview/Store/CodexReviewStore.swift) - [レビュー処理の登録と終了](../Sources/CodexReview/Store/ReviewStoreLifecycle.swift) -- [更新の実行と Monitor の再起動](../Tools/ReviewMonitor/CodexReviewMonitor/ReviewMonitorCodexUpdater.swift) -- [現在の停止確認](../Tools/ReviewMonitor/CodexReviewMonitor/CodexReviewMonitorApp.swift) +- [更新処理](../Tools/ReviewMonitor/CodexReviewMonitor/ReviewMonitorCodexUpdater.swift) +- [停止確認](../Tools/ReviewMonitor/CodexReviewMonitor/CodexReviewMonitorApp.swift) -## Store がキューと更新中の状態を所有する +## Store がキューと更新操作を持つ -新しい package や target は作らない。`CodexReviewStore` をレビューと runtime 状態の正本とする現在の構成を維持する。 +既存の package と target を使い、レビューと runtime の状態を Store に集約する。 -| 責務 | 所有者 | +| 処理・状態 | 担当 | | --- | --- | -| 更新予約、実行済みレビューの完了待ち、キュー、再開 | `CodexReviewStore` | -| ジョブの受付・実行開始・終端の保存 | 既存の履歴 coordinator と `CodexReviewPersistence` | -| app-server の終了・生成・初期化と設定の引き継ぎ | Store の runtime 差し替え処理と `CodexReviewHost` | -| 使用する CLI の解決とプロセス I/O | 既存の Host / AppServer 層 | -| 更新可否の確認、`codex update` の実行 | 既存の更新処理。Store に渡す依存として扱う | -| 起動時・8時間周期の確認、手動確認と確認結果 | `ReviewMonitorCodexUpdater`。設定画面も同じインスタンスを参照する | -| MCP の呼び出しと完了待機 | 既存の MCP adapter と Store | -| 選択肢、更新待ち・更新中・失敗の表示 | アプリと `ReviewUI`。Store の状態を表示する | +| 更新予約、レビュー完了待ち、キュー、再開 | `CodexReviewStore` | +| 受付・実行開始・終端の保存 | 履歴 coordinator と `CodexReviewPersistence` | +| app-server の終了・生成・初期化、設定の引き継ぎ | Store の runtime 差し替え処理と `CodexReviewHost` | +| CLI の解決とプロセス I/O | Host / AppServer 層 | +| 更新可否の確認、`codex update` の実行 | 既存の更新処理を Store へ渡す | +| 自動・手動確認とその結果 | `ReviewMonitorCodexUpdater`。設定画面とサイドバーで共有する | +| MCP 呼び出しと完了待機 | MCP adapter と Store | +| 更新待ち・進行・失敗の表示と操作 | アプリと `ReviewUI` | -アプリ側にレビューの一覧や再投入用の配列を持たせない。待機ジョブの対象・cwd・選択済みモデルは、受付時のレビュー要求から保持する。キューは同じジョブを参照し、UI 用のジョブを別途生成しない。 +キューは受付済みのジョブを参照し、対象・cwd・モデルを受付時の要求から保持する。 +アプリに再投入用の一覧や UI 専用ジョブを作らない。更新の段階も Store の一つの +操作で管理し、Updater に複製しない。 -更新待ちの内部状態は Store が持つ単一の更新操作にまとめる。`ReviewMonitorCodexUpdater` に同じ段階やレビュー残数を複製しない。 +### アプリから呼ぶ API -### アプリは更新操作を一度呼ぶ - -アプリ向けの利用例は次の形とする。名前は提案で、外部向けの汎用メンテナンス API は追加しない。 +設計時の利用例: ```swift try await store.updateCodex(when: .afterCurrentReviews) { @@ -91,68 +104,96 @@ try await store.updateCodex(when: .afterCurrentReviews) { } ``` -`updateCodex` は `CodexReviewStore` の ApplicationHostSupport SPI として提供する。タイミングは `afterCurrentReviews` と `immediately` の2つとし、同じ処理で待機と即時更新を扱う。キューの停止・再開や複数の完了通知を呼び出し側に要求しない。 - -`installer.install(plan)` は `codex update` の完了を返す依存である。テストではこの依存と backend transport を差し替え、Store のキューや runtime 差し替えを本番と同じ経路で動かす。 +`updateCodex` は ApplicationHostSupport SPI とし、タイミングは +`afterCurrentReviews` と `immediately` を用意する。呼び出し側は一度呼んで +結果を待ち、キューの停止・再開を個別に操作しない。汎用のメンテナンス API は +追加しない。 -## 更新はレビューの完了後に一度実行する +`installer.install(plan)` は `codex update` の完了を返す。テストではこれと +backend transport を差し替え、Store のキューと runtime の処理を本番と同じ +経路で確認する。 -更新操作は、受付済みで実行中のレビューについて、最終結果の保存と backend の後片付けまで待つ。既存の cleanup timeout は維持し、追加の無期限待機や「全未完了ジョブがゼロ」という条件は置かない。 +## 終了を確認してから更新する -その後、旧 app-server の終了を確認して `codex update` を実行する。更新中も MCP サーバー、セッション、ジョブ、待機中の呼び出しは存続する。更新後は既存の CLI 探索で実行ファイルを解決し、認証・設定を読み直して新しい runtime を公開する。公開成功後にキューを既存の worker 開始処理へ渡す。 +更新操作は実行中レビューの結果保存と backend の後片付けを待つ。 +既存の cleanup timeout を使い、待機ジョブまで含む「全未完了ジョブがゼロ」を +開始条件にしない。 -本案ではダウンロードだけを先行させない。`codex update` は取得とインストールをまとめて行うため、使用中の補助プログラムまで入れ替わる操作をレビューと並行させない。専用のダウンロード状態や二重の更新キャッシュも追加しない。 +旧 app-server の終了を確認した後で `codex update` を実行する。取得と +インストールを分離できないため、レビューと並行してダウンロードを先行させない。 +専用のダウンロード状態や二重のキャッシュも設けない。 -更新予約中にボタンを再度押しても、更新プロセスやキューのコピーを増やさない。MCP の待機が上限に達しても、同じ更新操作とジョブを維持する。 +更新後は CLI を再探索し、認証・設定を読み直して runtime を起動する。 +起動に成功したらキューを worker 開始処理へ渡す。ボタンを再度押した場合や +MCP の待機上限に達した場合も、同じ更新操作とジョブを維持する。 -### 失敗しても受け付けたジョブを失わない +### 失敗時も待機ジョブを保持する | 状況 | 動作 | | --- | --- | -| 更新に失敗したが、現在の CLI で app-server を起動できる | 更新エラーを表示し、その runtime でキューを再開する。旧版へ戻せたとは断定しない。 | -| 旧 app-server の終了を確認できない | インストールを始めない。状態と残ったプロセスを報告し、キューを保持する。 | -| 更新後の app-server 起動・認証・設定復元に失敗 | MCP とキューを保持してエラーを表示する。既存の再試行から復旧し、成功後に再開する。 | -| 待機ジョブの `review_cancel`、または所有セッションの明示終了 | app-server に送る前にそのジョブをキャンセルし、履歴と待機中の呼び出しへ結果を返す。 | -| アプリを明示的に終了 | 既存の終了処理に参加する。更新コマンドの実行途中なら、その結果が不明にならないよう完了を待つ。 | - -アカウント切替・サインアウト・手動再起動は更新操作と直列化する。既存の確認とキャンセルの契約を保ち、別アカウントへ保留要求を黙って移さない。通常の状態参照まで更新完了待ちにはしない。 - -## キューを履歴から消さず、実行開始時刻も区別する - -待機中も一覧とキャンセルの対象になるため、受付済みジョブを既存の履歴保存に載せる。受付時刻と実行開始時刻を分け、`queued` の段階では実行時間を計測しない。保存失敗は受付失敗として呼び出し元へ返し、メモリだけに受理済みジョブを残さない。 - -既存の履歴にキュー状態と実行開始の保存を追加し、古いデータは現在と同じ実行済み状態として読めるようにする。新たな保存用ファイルは作らない。 +| 更新失敗後に現在の CLI で起動できる | エラーを表示してキューを再開する。旧版へ戻ったとは扱わない | +| 旧 app-server の終了を確認できない | インストールを始めず、状態と残ったプロセスを報告してキューを保持する | +| 更新後の起動・認証・設定復元に失敗する | MCP とキューを維持し、既存の再試行で復旧後に再開する | +| 待機ジョブをキャンセルする、または所有セッションを明示終了する | app-server へ送る前にキャンセルし、履歴と待機呼び出しへ結果を返す | +| アプリを明示終了する | 終了処理に参加する。実行中の更新コマンドは結果が確定するまで待つ | -今回保証するのは、Monitor を継続稼働させたままの app-server 更新である。Monitor 自体のクラッシュや終了後に、以前の MCP セッションのジョブを自動実行する機能は追加しない。既存の復元契約に沿って中断した履歴を表示し、要求の重複実行を避ける。 +アカウント切替・サインアウト・手動再起動は更新と直列化する。既存の確認と +キャンセルを保ち、待機要求を別アカウントへ移さない。状態参照は更新完了を +待たずに使える。 -## 操作画面では「後でアップデート」を選べる +## 待機中も履歴とキャンセルの対象にする -レビュー中の確認には「後でアップデート」と、既存の「レビューを停止してアップデート」を用意する。「後で」は今回の更新を予約する選択肢で、単にダイアログを閉じる意味にはしない。 +受付時刻と実行開始時刻を分け、`queued` の間は実行時間を計測しない。 +受付履歴の保存に失敗したら要求を失敗として返し、メモリだけに受理済みジョブを +残さない。既存の履歴保存にキューと実行開始を加え、新しい保存ファイルは作らない。 +古い履歴は従来どおり実行済みとして読めるようにする。 -予約後はサイドバーに更新待ちを示し、新たなレビューは `Queued` と表示する。更新処理に進んだら更新中へ切り替え、完了後に通常表示へ戻す。実行中のレビューがなければ、そのまま更新へ進める。 +アプリを継続稼働させている間の更新を対象とする。クラッシュや終了後は既存の +復元処理で中断した履歴を表示し、以前の MCP セッションのジョブを自動実行しない。 -既存のウィンドウ、ログ、アカウント表示は保つ。待機・更新中・失敗・再開を `#Preview` と UI テストから確認できるようにする。 +## 画面と更新確認 -## 起動時と8時間ごとに確認し、設定画面からも確認できる +レビュー中は「後でアップデート」と「レビューを停止してアップデート」を提示する。 +前者は更新を予約する操作とし、予約後はサイドバーに更新待ち、新規ジョブに +`Queued` を表示する。実行中レビューがなければそのまま更新に進む。 +更新開始後は進行中の表示へ変え、完了後に通常表示へ戻す。 -自動確認は Monitor 起動時に一度、その後は起動から8時間、16時間、24時間という周期で行う。確認処理にかかった時間や、手動で確認した時刻を次回の起点にしない。スリープなどで予定時刻を過ぎた場合は復帰後に一度確認し、過ぎた回数だけ連続実行しない。 +既存のウィンドウ、ログ、アカウント表示を保ち、待機・更新中・失敗・再開を +`#Preview` と UI テストで確認する。 -アプリ全体で一つの確認処理を共有する。自動確認と手動確認が重なった場合は進行中の確認に合流する。Codex の入れ替え中に確認予定が来た場合も、同じ更新対象へ並行してコマンドを起動せず、更新後に一度確認する。 +自動確認は起動直後と、起動から8時間、16時間、24時間の時点で行う。 +確認にかかった時間や手動確認で周期をずらさない。スリープなどで時刻を過ぎたら +復帰後に一度確認し、過ぎた回数分を連続実行しない。 -設定画面に Codex のアップデート確認用 UI を追加する。手動ボタンは確認だけを行い、インストールやレビューの停止・キュー予約を始めない。表示する結果は「確認中」「更新あり」「最新」「このインストールでは確認できない」「確認失敗」を区別し、最終確認時刻とエラーも示す。CLI の選択失敗や未対応のインストール方法を「最新」と表示しない。 +自動・手動確認はアプリ全体で一つの処理を共有し、重なれば進行中の確認を待つ。 +インストール中に予定時刻が来たら更新後に一度確認する。設定画面を閉じても +周期を維持し、開くたびに監視タスクを作らない。CLI の起動時チェック設定で +Monitor の確認を停止しない。 -確認結果は既存のサイドバーの Update 表示にも反映する。設定画面を閉じても8時間周期は継続し、設定画面を開くたびに監視タスクを作らない。Monitor の確認周期は今回の要求を正本とし、CLI 用の起動時チェック設定を理由に Monitor の手動確認や周期確認を停止しない。 +設定画面の手動ボタンは確認だけを行う。インストール、レビュー停止、キュー予約は +始めない。「確認中」「更新あり」「最新」「確認できない」「確認失敗」を区別し、 +最終確認時刻とエラーを示す。CLI 選択の失敗や未対応のインストール方法を「最新」と +表示しない。結果はサイドバーの Update 表示にも反映する。 -## 3つの実装単位で検証する +## 実装と検証の順序 -1. **Kit のジョブ受付と実行開始を分離する。** キュー、履歴保存、キャンセル、同じ MCP 呼び出しの完了待機を実装する。実行の許可がある通常時には、既存の並行実行を保つ。 -2. **MCP を維持する更新操作を実装する。** 実行中レビューの完了待ち、旧 runtime の終了、更新依存の実行、新 runtime の公開、キュー再開を Store が一度の操作として所有する。 -3. **更新 UI と確認周期を接続して旧再起動経路を削除する。** 新しい選択肢、起動時・8時間ごとの確認、設定画面の手動確認を接続する。Codex 更新専用の Monitor 再起動ヘルパー・失敗起動引数を削除し、Preview と利用文書を更新する。 +1. Kit の受付と実行開始を分ける。キュー、履歴、キャンセル、MCP の完了待機を + 実装し、通常時の並行実行を保つ。 +2. MCP を維持する更新操作を実装する。レビュー完了待ち、runtime 終了、 + 更新、新 runtime の起動、キュー再開を Store が管理する。 +3. 更新 UI と確認周期を接続する。Monitor 再起動用ヘルパーと失敗起動引数を + 削除し、Preview と利用文書を更新する。 -親 Issue で順序と完了条件を管理し、1単位を1つの Ready PR にする。各 PR はローカル codex-review、リモートレビュー、CI を通してから main へ統合する。 +親 Issue で順序と完了条件を管理し、各単位を一つの Ready PR にする。 +ローカル codex-review、リモートレビュー、CI を通してから main へ統合する。 -回帰テストでは、更新予約とレビュー受付が同時に起こる順序、履歴保存中の要求、複数セッションのキュー、待機ジョブのキャンセル、更新・終了・再起動の失敗を確認する。既存の fake backend、transport、時計、gate を使い、時間経過への期待だけで同期しない。 +回帰テストは既存の fake backend、transport、時計、gate を使う。 +予約と受付の競合、履歴保存中の要求、複数セッション、待機ジョブのキャンセル、 +更新・終了・再起動の失敗を対象にする。固定時間の経過だけで同期しない。 -最終確認では、一つの MCP セッションでレビュー A を実行中に後回し更新を予約し、B と C を受け付ける。A の結果を返した後に更新が一度だけ実行され、同じセッション・ジョブ ID で B と C が完了することを確かめる。待機上限に届かない条件では、クライアントの再送・追加の待機ツール呼び出しが不要であることも検証する。 +最終確認では同じ MCP セッションで A を実行中に更新を予約し、B と C を受け付ける。 +A の結果を返した後、更新が一度だけ実行され、同じセッション・ジョブ ID で +B と C が完了することを確認する。540秒以内なら再送や追加の待機呼び出しは不要とする。 -更新チェックは手動時計で起動直後・8時間・16時間の実行を確認する。途中の手動確認で周期が変わらないこと、確認が重複しないこと、設定画面の表示結果とサイドバーが一致することも対象にする。 +手動時計で起動直後・8時間・16時間の確認を検証する。途中の手動確認が周期を +変えないこと、重複実行しないこと、設定画面とサイドバーの結果が一致することも確かめる。 diff --git a/Docs/mcp.md b/Docs/mcp.md index 875fdc3a..081806b6 100644 --- a/Docs/mcp.md +++ b/Docs/mcp.md @@ -1,204 +1,134 @@ -# MCP +# MCP reference -ReviewMonitor exposes Codex review over its app-managed MCP Streamable HTTP -endpoint. - -## Server Behavior - -- App-managed Streamable HTTP MCP endpoint at `http://localhost:9417/mcp` -- Multi-session -- Session-scoped review jobs -- One long-lived `codex app-server` backend process -- One shared internal transport to the backend process -- Review jobs run concurrently across sessions and within the same session - -## Lifecycle Responses - -`review_start`, `review_await`, `review_read`, and `review_cancel` return a -`lifecycle` object. Each `review_list.items` entry contains the same object. - -`lifecycle.status` is the broad job state: `queued`, `running`, `succeeded`, -`failed`, or `cancelled`. `lifecycle.terminal` is the authoritative terminal -classification and is `null` while the job is queued or running. Terminal -values have one of these shapes: - -- Completed: `{"kind":"completed"}` -- Failed: `{"kind":"failed","message":}` -- Interrupted: `{"kind":"interrupted","cause":}` - -An interruption `cause` has a `kind`, `source`, and `message`: - -- `requested`: `source` is `userInterface`, `mcpClient`, `sessionClosed`, or - `system`; `message` contains the cancellation reason. -- `server`: `source` is `null`; `message` may contain a server-provided reason. -- `transport`: `source` is `null`; `message` describes the transport failure. -- `previousProcessExit`: `source` and `message` are `null`. - -The current contract pairs `completed` with `status: "succeeded"`, a requested -interruption with `status: "cancelled"`, and the other terminal forms with -`status: "failed"`. - -`lifecycle.cancellation` records cancellation intent and its source. -`lifecycle.terminal` records the authoritative outcome. Do not infer an -interrupted terminal from `cancellation` alone. +ReviewMonitor hosts a Streamable HTTP MCP server at +`http://localhost:9417/mcp`. It shares one long-lived `codex app-server` process +across sessions. Reviews can run concurrently within a session and across +sessions, but each session can access only its own jobs. ## Tools -### `review_start` - -Runs a review through the shared long-lived `codex app-server` backend. - -Key inputs: +| Tool | Use | +| --- | --- | +| `review_start` | Start a review and wait for its result | +| `review_await` | Continue waiting for a queued or running review | +| `review_read` | Read a job snapshot and a page of logs | +| `review_list` | List jobs in the current session | +| `review_cancel` | Cancel a job in the current session | -- `cwd` -- `target` +### `review_start` -`target` uses the app-server review target model: +Pass `cwd` and a `target`. The target takes one of these forms: - `{"type":"uncommittedChanges"}` - `{"type":"baseBranch","branch":"main"}` - `{"type":"commit","sha":"abc1234","title":"Optional title"}` - `{"type":"custom","instructions":"Free-form review instructions"}` -Returns: - -- `jobId` -- `run` - - `reviewThreadId` - - `threadId` - - `turnId` - - `model` effective resolved review model -- `lifecycle` - - `status` - - `exitCode` - - `startedAt` - - `endedAt` - - `elapsedSeconds` - - `cancellable` - - `cancellation` when cancellation metadata is available - - `errorMessage` - - `terminal` authoritative terminal classification, or `null` before a - terminal result; see [Lifecycle Responses](#lifecycle-responses) -- `output` - - `summary` - - `review` - - `hasFinalReview` - - `lastAgentMessage` - - `reviewResult` parsed finding state (`hasFindings`, `noFindings`, or `unknown`) with title/body/location fields when available - -Notes: - -- `review_start` is the primary client flow. Every client waits up to 540 - seconds; if the job is still running, call `review_await` with the returned - `jobId`. -- A terminal result is returned after its history commit is durable. Backend - cleanup continues independently, and a later cleanup failure is available as - a developer diagnostic through `review_read` with `logFilter: "all"`. -- ReviewMonitor starts a job with its effective settings model. After thread - creation, it reports `thread/start.model` when available and otherwise keeps - that requested model. -- Use `review_read` to fetch paged, ordered `logs`. `rawLogText` is the - diagnostic/raw projection and is not a full log transcript. +The call waits up to 540 seconds, including time spent queued during a Codex +update. If the returned status is still `queued` or `running`, call +`review_await` with the returned `jobId`. + +The response contains `jobId`, `run`, `lifecycle`, and `output`, described +[below](#response-fields). ReviewMonitor starts with its settings model and +reports `thread/start.model` when the server supplies it. Otherwise, it keeps +the requested model. + +The store waits for the terminal history commit before returning the result. +Backend cleanup continues separately. A later cleanup failure appears in +`review_read` with `logFilter: "all"`. ### `review_await` -Waits for a running review job owned by the current MCP session. The wait is -bounded to 540 seconds so clients with fixed activity watchdogs can continue -waiting with another tool call. +Pass `jobId` or `jobID` for a job in the current session. Each call waits up to +540 seconds and returns the same fields as `review_start`. Call it again if the +job is still queued or running. -Inputs: +Use `review_read` for logs; start and await responses contain neither `logs` +nor `rawLogText`. -- `jobId` or `jobID` +### `review_read` -Returns the same lightweight shape as `review_start`: `jobId`, `run`, -`lifecycle`, and `output`. It does not include `logs` or `rawLogText`; use -`review_read` when log pages are needed. +Pass `jobId` or `jobID` to read a job independently of start or await. -If the job is still running after the bounded wait, call `review_await` again -with the same `jobId`. +| Optional argument | Meaning | +| --- | --- | +| `logOffset` | Zero-based offset; omitting it selects the latest page | +| `logLimit` | Page size, default `100`, maximum `500` | +| `logFilter` | `default` excludes command output and developer entries; `all` includes both | -### `review_read` +The response adds `logs`, `logsPage`, and `rawLogText` to the common fields. +Before paging, the server folds grouped replacements and deltas into their +current values. Developer entries have `audience: "developer"`; product entries +omit `audience`. -Reads the current or final state of a review job owned by the current MCP session. -Use this to fetch log pages or to refresh a job snapshot independently of the -bounded `review_start` / `review_await` flow. - -Inputs: - -- `jobId` or `jobID` - -Optional paging inputs: - -- `logOffset` 0-based log page offset. If omitted, `review_read` returns the - latest page. -- `logLimit` page size, default `100`, max `500` -- `logFilter` `default` excludes command output and developer-only entries; - `all` includes both - -Returns: - -- `jobId` -- `run` -- `lifecycle` -- `output` -- `logs` paged read projection. Grouped replacement/delta entries are folded - into their current value before paging. Developer-only entries returned by - `logFilter: "all"` include `audience: "developer"`; product entries omit - `audience`. -- `logsPage` - - `total` - - `offset` - - `limit` - - `returned` - - `hasMoreBefore` - - `hasMoreAfter` - - `previousOffset` - - `nextOffset` -- `rawLogText` diagnostic/raw projection, not a full transcript +`logsPage` contains `total`, `offset`, `limit`, `returned`, `hasMoreBefore`, +`hasMoreAfter`, `previousOffset`, and `nextOffset`. Use the offsets to fetch +adjacent pages. `rawLogText` contains raw diagnostic text, not a full transcript; +use `logs` for ordered log entries. ### `review_list` -Lists review jobs owned by the current MCP session. +Lists jobs in the current session. + +| Optional argument | Meaning | +| --- | --- | +| `cwd` | Filter by working directory | +| `statuses` | Filter by lifecycle status | +| `limit` | Number of jobs, default `20`, maximum `100` | -Optional inputs: +Each entry in `items` contains `jobId`, `cwd`, `targetSummary`, `run`, +`lifecycle`, and `output`. -- `cwd` -- `statuses` -- `limit` default `20`, max `100` +### `review_cancel` -Returns: +Pass `jobId` to cancel one job, or use `cwd` and `statuses` to select jobs in the +current session. A working directory can match more than one job. -- `items` - - `jobId` - - `cwd` - - `targetSummary` - - `run` - - `lifecycle` - - `output` +The response includes cancellation source and message when available. +Cancellations from the app use `source: "userInterface"`. -### `review_cancel` +## Response fields + +Start, await, read, and cancel responses share these fields. List entries use +the same `run`, `lifecycle`, and `output` objects. -Cancels a review job owned by the current MCP session. +| Object | Fields | +| --- | --- | +| `run` | `reviewThreadId`, `threadId`, `turnId`, `model` (effective review model) | +| `lifecycle` | `status`, `exitCode`, `startedAt`, `endedAt`, `elapsedSeconds`, `cancellable`, `cancellation`, `errorMessage`, `terminal` | +| `output` | `summary`, `review`, `hasFinalReview`, `lastAgentMessage`, `reviewResult` | -Inputs: +`reviewResult` describes parsed findings as `hasFindings`, `noFindings`, or +`unknown`. Findings include title, body, and location fields when available. -- exact: - - `jobId` -- selector: - - `cwd` - - `statuses` +### Lifecycle responses -Notes: +`status` is `queued`, `running`, `succeeded`, `failed`, or `cancelled`. +`terminal` is `null` while the job is queued or running. Once it ends, the +terminal object records its outcome: -- `cwd` is a search key, not a unique identifier. -- Without `jobId`, `review_cancel` searches only the current MCP session. -- Responses include `lifecycle.cancellation.source` and `lifecycle.cancellation.message` when cancellation metadata is available. UI-triggered cancellations use `source: "userInterface"`. +| Outcome | Terminal object | Status | +| --- | --- | --- | +| Completed | `{"kind":"completed"}` | `succeeded` | +| Failed | `{"kind":"failed","message":}` | `failed` | +| Interrupted | `{"kind":"interrupted","cause":}` | `cancelled` for requested interruption; otherwise `failed` | -## Discovery Resources +An interruption cause contains `kind`, `source`, and `message`: -ReviewMonitor exposes onboarding/discovery resources over MCP. Clients can use `resources/list` and `resources/read` to inspect supported review flows without relying on the README. +| `kind` | `source` | `message` | +| --- | --- | --- | +| `requested` | `userInterface`, `mcpClient`, `sessionClosed`, or `system` | Cancellation reason | +| `server` | `null` | Server reason, when supplied | +| `transport` | `null` | Transport failure | +| `previousProcessExit` | `null` | `null` | -Useful resources: +`cancellation` records a cancellation request and its source. A request may +arrive before the review's final outcome, so use `terminal` to determine how +it ended. + +## Help resources + +Use `resources/list` and `resources/read` to read help from the server: - `codex-review://help/overview` - `codex-review://help/tools/review_start` @@ -208,14 +138,14 @@ Useful resources: - `codex-review://help/targets/commit` - `codex-review://help/targets/custom` -## Resource Templates - -ReviewMonitor also exposes MCP resource templates for tool-specific and target-specific help. Clients can discover them via `resources/templates/list`. +`resources/templates/list` also provides templates for tool and target help. -## Runtime Files +## Runtime files -ReviewMonitor uses `~/.codex_review` as its dedicated Codex home. +ReviewMonitor keeps its Codex runtime files in `~/.codex_review`: -- `config.toml` stores backend settings for this dedicated home -- `review_mcp_endpoint.json` records the current HTTP/SSE endpoint -- `review_mcp_runtime_state.json` records internal server/runtime ownership state +| File | Contents | +| --- | --- | +| `config.toml` | Backend settings | +| `review_mcp_endpoint.json` | Current HTTP/SSE endpoint | +| `review_mcp_runtime_state.json` | Server and runtime ownership state | diff --git a/Docs/releases.md b/Docs/releases.md index 76b1f368..6dad71cf 100644 --- a/Docs/releases.md +++ b/Docs/releases.md @@ -1,22 +1,12 @@ -# Releasing CodexReviewMonitor +# Release and build guide -This guide is for maintainers preparing signed release DMGs or validating the -release build. For app installation and client setup, see the -[README](../README.md#quick-start). Run the shell commands below from the -repository root. +For installation and MCP setup, see the [README](../README.md#quick-start). +Run the commands in this guide from the repository root. -## Publish a Release +## Publish a release -After the [one-time signing setup](#one-time-signing-setup), approve the version, -release notes, and source commit before starting publication. CI runs checks and -the build, then waits for your approval of the `release-signing` Environment. -After you approve in GitHub, CI performs Developer ID signing, notarization, -asset upload, and publication automatically. -A successful run publishes the existing draft without changing its title or notes. -No local process or LLM needs to watch the run. - -To create the draft and start CI in one operation, save the approved notes in a -UTF-8 file and run: +Complete the [signing setup](#one-time-signing-setup), then approve the version, +notes, and source commit. Save the approved notes in a UTF-8 file and run: ```bash python3 scripts/prepare_release.py start \ @@ -25,75 +15,84 @@ python3 scripts/prepare_release.py start \ --notes-file /path/to/release-notes.md ``` -Add `--prerelease` for a prerelease. The command targets the current remote `main` -commit, creates a draft with the supplied notes, dispatches the workflow, and -returns immediately. It does not create a tag or wait for CI. If dispatch cannot -be confirmed, the draft remains available; check Actions before retrying the -workflow to avoid starting it twice. +Add `--prerelease` for a prerelease. The command creates a draft targeting the +current remote `main` commit, starts CI, and returns. It leaves tag creation to +publication. If workflow dispatch is uncertain, check Actions before retrying; +the draft remains available. + +CI checks and builds the app, then waits for approval of the `release-signing` +Environment in GitHub. After approval, it signs with Developer ID, notarizes, +verifies and uploads the assets, and publishes the same draft. The draft's title +and notes are preserved. Publication continues without a local process watching +the run. + +### Use an existing draft -To use a draft already prepared in GitHub, save its version, title, notes, and -prerelease setting with `main` as the target. Then open +Save the version, title, notes, and prerelease setting in GitHub, with `main` as +the target. Open [Publish Release](https://github.com/lynnswap/CodexReviewKit/actions/workflows/release.yml), -choose **Run workflow** on `main`, and enter the draft's tag. This requests -publication after CI succeeds and you approve signing. -[Draft creation and editing do not trigger Actions](https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows#release), -so this one dispatch is necessary. An API-created draft can target the full -workflow commit SHA instead of `main`; a different commit is not silently substituted. - -The workflow pins the draft to its full source SHA before building. All CI checks -must pass before signing. Signing runs separately from the build, uses native -Apple tools and an ephemeral keychain, and does not execute the app. The final -job verifies and uploads the DMG, `release-info.json`, and `SHA256SUMS`, then -publishes that same draft. GitHub creates the tag at publication time. -Only those three verified assets may be attached at publication; remove any -unintended draft attachments before retrying a failed publication. - -Failures before publication leave the release as a draft. Rerun failed jobs to -reuse successfully built and signed artifacts. Matching uploads are retained; -different bytes under an existing asset name stop publication instead of being -overwritten. If publication succeeded but its confirmation failed, rerunning the -publish job confirms the same assets and tag without changing the public release. -If rerunning the entire workflow creates different artifacts, inspect -the draft's existing assets before removing them and retrying. Keep the tag, -target commit, and prerelease setting unchanged while a run is active; title and -release-note edits are preserved. Notarization can continue at Apple after a -workflow timeout; diagnostics include its submission ID. - -The numeric part of the tag sets the app's marketing version: `v1.2.3-beta.1` -produces version `1.2.3`, with the full tag retained in the filename and metadata. -The workflow run number sets the build version. Retrying the same run keeps its -build number. +choose **Run workflow** on `main`, and enter the draft's tag. +[Creating or editing a draft does not trigger Actions](https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows#release), +so dispatch the workflow once. An API-created draft can instead target the full +workflow commit SHA; the workflow rejects a different commit. + +### Assets and retries + +The workflow pins the draft to its full source SHA before building. Checks pass +before signing, which runs separately with native Apple tools and a temporary +keychain. The signing job does not run the app. The publication job verifies +these three assets: + +- The DMG +- `release-info.json` +- `SHA256SUMS` + +Only those assets may be attached when the draft is published. Remove unintended +attachments before retrying. GitHub creates the tag at publication time. + +A failure before publication leaves the draft unpublished. Rerun failed jobs to +reuse completed build and signing artifacts. An uploaded asset with matching +bytes is kept; different bytes under the same name stop publication. If +publication succeeded but confirmation failed, rerunning the publication job +checks the existing assets and tag without changing the release. + +Rerunning the whole workflow can produce different artifacts. Inspect existing +draft assets before removing them and retrying. While a run is active, keep its +tag, source commit, and prerelease setting fixed. You can edit the title and +notes. Apple may continue notarization after a workflow timeout; use the +submission ID in the diagnostics to identify it. + +The tag's numeric part sets the marketing version: `v1.2.3-beta.1` becomes +`1.2.3`. Filenames and metadata retain the full tag. The workflow run number is +the build version, and a retry of the same run keeps that number. ## One-time signing setup -Use **Settings → Environments → release-signing** for these values. Its branch -policy must allow only the `main` branch, with a required reviewer for signing. -Leave **Prevent self-review** off when the maintainer starting the workflow also -approves it. The maintainer approves this Environment in GitHub; CI publishes -automatically after the approved signing job and all checks succeed. +In **Settings → Environments → release-signing**, allow only `main` and require +a reviewer. Leave **Prevent self-review** off if the maintainer starting the run +will also approve signing. CI publishes after approval and successful checks. | Environment secret | Value | | --- | --- | -| `DEVELOPER_ID_P12_BASE64` | Base64 encoding of a password-protected `.p12` containing only the intended Developer ID Application certificate and private key | -| `DEVELOPER_ID_P12_PASSWORD` | The `.p12` export password | -| `NOTARY_API_PRIVATE_KEY` | The full contents of the App Store Connect Team API key's `.p8` file | +| `DEVELOPER_ID_P12_BASE64` | Base64-encoded, password-protected `.p12` with the intended Developer ID Application certificate and private key | +| `DEVELOPER_ID_P12_PASSWORD` | Export password for the `.p12` | +| `NOTARY_API_PRIVATE_KEY` | Full contents of the App Store Connect Team API key's `.p8` file | | Environment variable | Value | | --- | --- | -| `APPLE_TEAM_ID` | The Apple Developer Team ID matching the signing certificate | -| `NOTARY_API_KEY_ID` | The App Store Connect API key ID | -| `NOTARY_API_ISSUER_ID` | The issuer UUID for the Team API key | - -Export the intended **Developer ID Application** signing identity, including its -private key, as a password-protected `.p12`. An `Apple Development` certificate -does not work for this distribution channel. Keep a secure backup of the signing -identity. For notarization, create a dedicated **Team API key** with the -**Developer** role; this role permits notarization but is not limited to it or to -this app. See [Apple's Developer ID guide](https://developer.apple.com/help/account/certificates/create-developer-id-certificates/) +| `APPLE_TEAM_ID` | Team ID matching the signing certificate | +| `NOTARY_API_KEY_ID` | App Store Connect API key ID | +| `NOTARY_API_ISSUER_ID` | Team API key issuer UUID | + +Export one valid **Developer ID Application** identity, including its private +key, to the password-protected `.p12`, and keep a secure backup. An +`Apple Development` certificate cannot sign this distribution. For +notarization, create a dedicated **Team API key** with the **Developer** role. +That role also permits operations beyond notarization and this app. See +[Apple's Developer ID guide](https://developer.apple.com/help/account/certificates/create-developer-id-certificates/) and [API key management](https://developer.apple.com/help/app-store-connect/get-started/app-store-connect-api/). -With an authenticated GitHub CLI, secrets can be uploaded from local files -without putting their contents in command arguments: +With GitHub CLI authenticated, upload secrets from local files: ```bash base64 < /secure/path/DeveloperID.p12 | gh secret set DEVELOPER_ID_P12_BASE64 \ @@ -104,33 +103,32 @@ gh secret set NOTARY_API_PRIVATE_KEY \ --repo lynnswap/CodexReviewKit --env release-signing < /secure/path/AuthKey.p8 ``` -The password command prompts for its value. Enter the three non-secret variables -in the Environment's Variables section. The signing step imports the identity -into a temporary keychain, verifies its team and certificate type, and removes -the keychain and decoded key files when it finishes. The `.p12` must contain only -one valid signing identity. Keep credentials out of repository files and build -artifacts; update or revoke them through Apple and GitHub when needed. +The password command prompts for its value. Add the three variables in the +Environment's Variables section. The signing step checks the imported +certificate's team and type, then removes the temporary keychain and decoded +key files when it finishes. Keep credentials out of the repository and build +artifacts; update or revoke them through Apple and GitHub. -## Release Build Validation +## Release build validation -Maintainers can build a validation DMG entirely on GitHub Actions. Open +Open [Release Build](https://github.com/lynnswap/CodexReviewKit/actions/workflows/release-build.yml), -choose **Run workflow** on `main`, and enter a version label such as -`v0.0.0-validation`. The same build also runs for pushes to `main`. +choose **Run workflow** on `main`, and enter a label such as +`v0.0.0-validation`. The build also runs on pushes to `main`. -The workflow builds the selected commit with the runner's default Xcode, creates -the DMG without Finder or Apple credentials, and verifies the mounted app. -Download the DMG, `build-info.json`, and `SHA256SUMS` from the run's artifact. The metadata records -the source commit, version label, Xcode version, and workflow run. Artifacts are -retained for seven days. The numeric part of the version label sets the app's -marketing version; the workflow run number sets its build version. +The workflow uses the runner's default Xcode, packages the selected commit +without Finder or Apple credentials, and verifies the mounted app. Download the +DMG, `build-info.json`, and `SHA256SUMS` from its artifact, retained for seven +days. The metadata records the commit, version label, Xcode version, and run. +The label's numeric part sets the marketing version; the run number sets the +build version. -These artifacts are for build and packaging validation. The app is ad-hoc signed; -the DMG is not Developer ID signed or notarized. The workflow creates no tag or -GitHub Release. Use the signed and notarized public release for installation. +Validation artifacts use an ad-hoc app signature. The DMG has no Developer ID +signature or notarization, and the workflow creates no tag or GitHub Release. +Use a signed public release for installation. -To run the same packaging locally, create a Python 3.10 or newer virtual -environment and install the pinned DMG tools: +For the same packaging locally, prepare Python 3.10 or newer and the pinned +DMG tools: ```bash python3 -m venv .build/release-tools @@ -141,17 +139,49 @@ scripts/build-release.sh --version v0.0.0-validation scripts/package-release.sh --version v0.0.0-validation ``` -Local validation defaults to build number `1`; pass `--build-number` to the build -script to use another positive integer. +Local validation uses build number `1`. Pass `--build-number` to the build +script for another positive integer. + +## Local builds + +For a local app build, use an Apple silicon Mac with Xcode 26.4 or newer and +Python 3.10 or newer: + +```bash +python3 scripts/build_review_monitor.py +``` + +The script creates `dist/CodexReviewMonitor_yymmdd_hhmm.dmg`, named with the local +build-start time. It applies an ad-hoc hardened-runtime signature and verifies +the DMG contents. Caches stay in `.build`; the first run prepares the pinned +DMG tools in `.build/release-tools` and may download locked package dependencies. + +ReviewMonitor can stay open during the build. When reviews finish, quit the +app, open the DMG, drag the app to Applications, choose **Replace**, and relaunch. +You can replace the app without deleting it first. The script leaves installed +apps alone. Builds started in the same minute replace the same DMG only after +the new image passes validation; older filenames are kept. + +The ad-hoc signature is for local use. To select an approved local identity, +pass it explicitly: + +```bash +python3 scripts/build_review_monitor.py \ + --signing-identity 'Apple Development: Developer Name (TEAMID)' +``` + +The script uses that identity or fails; it never falls back to another one. +Local signing does not notarize the app or make it suitable for redistribution. +Device-management policy can still prohibit it. The script leaves Gatekeeper +and quarantine metadata in place. The app requires macOS 26 or newer. ## Repository protection -The `main` ruleset requires a pull request, resolved review threads, and passing -GitHub Actions checks against the current base branch. Deletion and force pushes -are blocked. No additional human approval is required, allowing a solo maintainer -to merge a reviewed PR. CI runs for documentation-only changes too, so required -checks can finish on every PR. +The `main` ruleset requires a PR, resolved review threads, and passing GitHub +Actions checks against the current base branch. It blocks deletion and force +pushes. A solo maintainer can merge a reviewed PR without another human approval. +CI also runs for documentation changes so required checks can finish on every PR. -The `release-signing` Environment allows only the `main` branch. The validation -workflow does not use this Environment or Apple secrets. The signing workflow -also binds its input artifact ID and file digest to the build in the same run. +Only `main` can use `release-signing`. Build validation uses neither that +Environment nor Apple secrets. Signing checks that its input artifact ID and +file digest belong to the build in the same workflow run. diff --git a/Docs/review-history-persistence-design-2026-08-29.md b/Docs/review-history-persistence-design-2026-08-29.md index 1390e54f..d1241548 100644 --- a/Docs/review-history-persistence-design-2026-08-29.md +++ b/Docs/review-history-persistence-design-2026-08-29.md @@ -1,158 +1,98 @@ -# Review history persistence design (2026-08-29) +# Review history persistence design -Status: Re-gated after adversarial review; implementation in progress +Design and validation record begun on 2026-08-29. The measurements and delivery +status below belong to that implementation, not the current checkout. See +[architecture.md](architecture.md) for the current target structure. -| Item | Value | +| Item | Recorded value | | --- | --- | +| Status | Re-gated after adversarial review; implementation in progress | | Integration branch | `codex/persist-review-history` | | Baseline | `22b1e975015b0bf24b45dad669a91c8b52fd8d2c` | | Target base | `main` | | Database framework | SQLiteData 1.11.2 | | Package baseline | Swift tools 6.3 / Swift language mode 6 | -| Local validation toolchain | Xcode 27.0 / Swift 6.4 | -| CI compatibility toolchain | Latest stable Xcode 26 runner selected by `.github/workflows/ci.yml` | - -This document is the design contract and progress ledger for durable ReviewMonitor -history. If implementation requires another owner, schema, lifecycle, failure -semantic, or MCP authorization policy, update this document before changing code. - -## 1. Scope contract - -### Outcome - -ReviewMonitor restores application-wide review history after relaunch without -persisting the live transcript. A restored row preserves its review identity, -workspace and manual order, target, effective model, lifecycle, canonical result, -and structured findings. Selecting a restored review renders a compact detail from -those semantic fields. - -Review manual order is independent of the workspace that executed the review. -Repository sections may combine a primary checkout and linked worktrees into one -visible list, so every review in that section participates in one reorder lane while -its immutable `cwd` continues to describe execution provenance. Existing databases -are normalized once from workspace order followed by workspace-local review order, -which preserves their pre-migration visible order. - -The feature is complete when: - -1. A succeeded, failed, or cancelled review remains one sidebar row after a clean - app restart. -2. The row retains its target, model, exact known duration, terminal cause, final - review, and findings. -3. A process-abandoned queued/running record is restored as - `.interrupted(.previousProcessExit)` and never appears live or cancellable. - Its unknown end time is not guessed. -4. History remains readable while the MCP runtime is starting, stopped, or failed. -5. A new MCP session cannot list, read, await, or cancel records restored from a - previous process. -6. Database open, migration, decode, or write failure is visible at the Store - boundary and blocks new review admission instead of silently using empty or - non-durable history. -7. A rebuilt app passes an isolated end-to-end run: launch on a dedicated MCP - port and history path, complete a real review, terminate, relaunch, and verify - the restored sidebar/detail/findings state. - -### Compatibility - -- Keep the four existing library products and their public source surface. -- Keep the five MCP tool names, schemas, response fields, and session-local - authorization behavior. -- No migration of a previously shipped review-history database is required; there - is no current durable history owner. -- Current account/settings/runtime persistence remains unchanged. -- Local commits, push, and the requested Ready PR are authorized for this task. - -### Non-goals - -- Full transcript or app-server event replay. -- Cross-process review resumption from thread/turn identifiers. -- Reproducible source archives, working-tree snapshots, or diff storage. -- CloudKit synchronization. -- Persisting credentials, account secrets, raw JSON-RPC, reasoning, command output, - tool results, developer diagnostics, or streaming deltas. -- Making previous-process history readable through a newly initialized MCP session. - -## 2. Phase 1 findings at the baseline - -### Measurements - -- `CodexReview`: 225 `public`, 872 `package`, 8 `open` tokens. -- `CodexReviewHost`: 45 `public`, 99 `package`, 10 `open` tokens. -- `ReviewUI`: 9 `public`, 2 `package`, 2 `open` tokens. -- No `#if canImport` / `#if os` source gates. -- Relevant largest files before migration: - - `LiveCodexReviewStoreBackend.swift`: 3,701 lines. - - `CodexReviewStoreReviews.swift`: 2,685 lines. - - `ReviewMonitorSidebarViewController.swift`: 2,672 lines. - - `CodexReviewStore.swift`: 1,709 lines. - - `CodexReviewJob.swift`: 1,004 lines. - -### Numbered findings - -#### F1 — Durable review membership has no owner (confirmed) - -`CodexReviewStore` owns process-local workspaces/jobs and its seed contains only -account/settings state. Relaunch constructs a new Store with no review records. - -#### F2 — The current aggregate mixes semantic state with transient projection (confirmed) - -`CodexReviewJob` contains canonical lifecycle/output alongside raw log entries, -incremental message assembly, rendered text projections, log revisions, and mutation -hints. Encoding the class would persist duplicate and runtime-only state. - -#### F3 — Exact review target is discarded after admission (confirmed) - -The validated `Start.Request.target` reaches the worker, while the job retains only -`targetSummary`. History must record the typed validated target at admission; it -must not parse the display string later. - -#### F4 — The current 256 KiB limit is not a durable log bound (confirmed) - -The cap excludes multiple log kinds and metadata payloads. Persisting -`ReviewLogEntry` is not a bounded-history design. - -#### F5 — The detail UI needs an explicit compact-history projection (confirmed) - -The selected detail renders `job.logEntries`, while workspace findings render -`core.output.reviewResult`. Restored history must build a canonical final/error/ -cancellation entry from semantic history rather than store rendered projections. - -#### F6 — Runtime availability currently hides otherwise valid history (confirmed) - -The sidebar selects its unavailable state before considering existing jobs when the -server is starting, stopped, or failed. Durable history must remain visible and use -the status accessory for runtime/history health. - -#### F7 — MCP authorization and application history have different lifetimes (confirmed) - -MCP read/list/cancel filter by the current transport session. Persisted history is -application-wide UI data and must restore with a non-live session identity. - -#### F8 — The existing history URL helper is not an independent live seam (confirmed) - -`PreparedRecoveryEnvironment.withHistoryDatabaseURL` belongs to an unused capability -graph that also owns a replacement Codex home, login staging, and saved accounts. -Activating the graph only for history would expand this change into runtime-home -migration. The history database therefore gets a dedicated Application Support -location owner, and the unused helper is removed so there is one live path owner. - -## 3. Target graph and owner map - -### Considered structures - -1. **Internal `CodexReviewPersistence` target (selected).** - It owns SQLiteData schema, migrations, queries, transactions, retention, and DB - close. `CodexReview` owns the semantic port/records; `CodexReviewHost` owns the - production URL and composition. This keeps SQLiteData out of domain and UI source. -2. Put SQLiteData inside `CodexReviewHost`. - This avoids one target but adds schema/query ownership to the existing 3,701-line - runtime adapter and cannot enforce the storage boundary. -3. Put SQLiteData inside `CodexReview`. - This makes the semantic target own a concrete I/O framework and lets persistence - types spread into the Store. It also makes preview/testing selection less explicit. - -The selected same-package internal target is justified by a real outbound-adapter -dependency boundary. It is not a new product or separately versioned package. +| Local validation | Xcode 27.0 / Swift 6.4 | +| CI compatibility | Latest stable Xcode 26 runner selected by `.github/workflows/ci.yml` | + +## What history preserves + +ReviewMonitor restores review history after relaunch. Each row keeps its ID, +workspace, manual order, typed target, effective model, lifecycle, final review, +and structured findings. The detail view derives a compact display from those +fields. Transcripts stay in memory. + +Manual review order applies across the app. A repository section can combine a +primary checkout with linked worktrees, while each review keeps its original +`cwd`. Existing databases migrate by sorting workspaces first, then the previous +workspace-local review order, preserving the visible order. + +Acceptance requires: + +1. A completed, failed, or cancelled review returns as one sidebar row after a + clean restart, with its target, model, known duration, terminal cause, result, + and findings. +2. A queued or running row left by the previous process becomes + `.interrupted(.previousProcessExit)`. It has no invented end time and cannot + appear live or cancellable. +3. History stays readable while the MCP runtime starts, stops, or fails. A new + MCP session cannot list, read, await, or cancel previous-process records. +4. Database open, migration, decode, and write failures are visible at the store + and block new review admission. The database is retained. +5. An isolated app E2E run completes a real review, terminates, relaunches, and + verifies the restored sidebar, detail, and findings. + +The four library products and their public source surface stay compatible, as +do the five MCP tool names, schemas, response fields, and session access rules. +Account, settings, and runtime persistence stay unchanged. At the baseline, +there was no shipped history database requiring migration. The original task +also authorized local commits, push, and a Ready PR. + +History excludes transcripts, raw JSON-RPC, reasoning, command output, tool +results, diagnostics, streaming deltas, credentials, account secrets, finding +`rawText`, and rendered projections. This work does not add event replay, +cross-process review resumption, source archives, working-tree snapshots, +diffs, or CloudKit synchronization. Thread and turn IDs cannot authorize or +resume a review after relaunch. + +## Problems at the baseline + +| Finding | Problem | Design response | +| --- | --- | --- | +| F1 | Store seeds contain accounts and settings, but no durable review membership | Load records through a history port and SQLite adapter | +| F2 | `CodexReviewJob` mixes results with logs, message assembly, rendering, revisions, and mutation hints | Persist semantic records instead of encoding the job | +| F3 | Admission passes the typed target to the worker but retains only `targetSummary` | Store the validated target without parsing its display text | +| F4 | The 256 KiB cap excludes some log kinds and metadata | Keep the final-result bound; omit a generic log table | +| F5 | Detail reads `job.logEntries`, while findings read `core.output.reviewResult` | Derive compact final, error, or cancellation entries from history | +| F6 | Sidebar runtime-unavailable state hides existing jobs | Keep history visible and show runtime health in the status accessory | +| F7 | MCP sessions and app history have different lifetimes | Restore rows with a non-live session identity and keep MCP filters | +| F8 | `PreparedRecoveryEnvironment.withHistoryDatabaseURL` also depends on replacement-home, login, and account staging | Give history its own Application Support location and remove the unused helper | + +Recorded source measurements: + +| Target | `public` | `package` | `open` | +| --- | ---: | ---: | ---: | +| `CodexReview` | 225 | 872 | 8 | +| `CodexReviewHost` | 45 | 99 | 10 | +| `ReviewUI` | 9 | 2 | 2 | + +There were no `#if canImport` or `#if os` source gates. The largest relevant files +were `LiveCodexReviewStoreBackend.swift` (3,701 lines), +`CodexReviewStoreReviews.swift` (2,685), +`ReviewMonitorSidebarViewController.swift` (2,672), +`CodexReviewStore.swift` (1,709), and `CodexReviewJob.swift` (1,004). + +## Owners and dependencies + +`CodexReviewPersistence` is an internal target in the same package. It owns +SQLiteData schema, migrations, queries, transactions, retention, and close. +`CodexReview` defines the records and persistence interface, and +`CodexReviewHost` supplies the production location and adapter. This keeps +SQLiteData out of the store and UI without adding a product or versioned package. + +Putting storage in `CodexReviewHost` would add schema and query work to the +3,701-line runtime adapter. Putting it in `CodexReview` would couple review +behavior to concrete database I/O and make preview and test selection less clear. ```text ReviewUI ───────────────────────────────▶ CodexReview @@ -163,53 +103,51 @@ CodexReviewHost ─────────┬─────────── ReviewMonitor.app ─────────────────────▶ CodexReviewHost + ReviewUI ``` -Responsibilities: - -- `CodexReview`: owns live review semantics, application-wide manual review order, - and the history persistence contract. -- `CodexReviewPersistence`: stores and restores semantic review records in SQLite. -- `CodexReviewHost`: supplies the owner-only production database location and concrete adapter. -- `ReviewUI`: renders Store state, defines repository-section membership, and - forwards the exact workspace scope of history deletion/reorder intent. -- `CodexReviewMCPServer`: keeps current-session authorization over Store commands. - -### Resource lifecycle - -```text -ReviewMonitor composition - -> prepare owner-only Application Support directory - -> construct ReviewHistoryDatabase with its exact URL - -> inject persistence port into CodexReviewStore - -> first Store load opens/migrates the explicit DatabasePool and orphan-finalizes history - -> runtime starts - -> review admission persists a running header before backend dispatch - -> worker finalization commits terminal result + findings + retention - -> application shutdown drains review workers - -> Store synchronizes terminal snapshots and ordering - -> history database closes - -> application termination replies -``` - -Runtime restart/account switch does not close history. Application shutdown is a -separate Store operation from runtime `stop()`. - -`CodexReviewStore` owns three internal linearization mechanisms: - -- `HistoryStartReceipt` captures validated target, model, job/workspace order, - session, and Store work admission before the start-header write. Pending receipts - are visible to session/runtime close. After the write it revalidates the same - receipt; stale starts are terminalized durably without backend dispatch. -- One `HistoryTerminalReceipt` per live job owns the first terminal snapshot and - exactly one durable commit. Worker completion, cancel response, runtime detach, - waiter resumption, and application shutdown join that receipt. -- `ReviewHistoryMutationCoordinator` executes database mutation and MainActor apply - in one ordinal lane. Reorder, terminal retention, and explicit delete cannot - apply results in a different order from their database commits. - -## 4. Semantic surface - -All new declarations are `package` unless an existing public API requires otherwise. -No SQLiteData or GRDB type appears outside `CodexReviewPersistence`. +| Owner | Responsibility | +| --- | --- | +| `CodexReview` | Live review behavior, app-wide manual order, and history contract | +| `CodexReviewPersistence` | Save and restore review records in SQLite | +| `CodexReviewHost` | Owner-only database location and live adapter | +| `ReviewUI` | Render store state, group repository sections, and pass workspace scope for deletion and reordering | +| `CodexReviewMCPServer` | Restrict commands to the current session | + +Update this design before changing an owner, schema, lifecycle, failure +behavior, or MCP access policy. + +### Lifetime and write order + +At composition, the host prepares the owner-only Application Support directory +and injects a database with its exact URL. The first store load opens and +migrates the database, finalizes abandoned rows, and restores history before +accepting reviews. Admission saves a start header before dispatching backend +work. Finalization saves the terminal result, findings, and retention changes +in one transaction. + +Application shutdown drains workers, synchronizes terminal snapshots and order, +then closes the database before replying to termination. Runtime `stop()`, +restart, and account switching leave history open. + +The store coordinates three kinds of work: + +- `HistoryStartReceipt` captures target, model, job and workspace order, session, + and admission before the write. Session and runtime close can see pending + receipts. After saving, the store rechecks the receipt; a stale request gets a + durable terminal result without backend dispatch. +- `HistoryTerminalReceipt` holds the first terminal snapshot and one commit per + live job. Worker completion, cancellation responses, runtime detach, waiters, + and shutdown all wait for that commit. +- `ReviewHistoryMutationCoordinator` runs each database mutation and its + MainActor state change in the same order. Reorder, retention, and deletion + therefore update the store in database commit order. + +The store retains those tasks and receipts until shutdown has waited for them. +Database writes alone cannot establish this ordering if their results are +applied independently on the MainActor. + +## Persistence interface + +New declarations use `package` access unless an existing public API requires +otherwise. SQLiteData and GRDB types stay in `CodexReviewPersistence`. ```swift package protocol ReviewHistoryPersistence: Sendable { @@ -249,220 +187,165 @@ extension CodexReviewStore { } ``` -The persistence boundary has phase-specific immutable values: +Immutable Sendable values cross this interface: -- `StartedReviewRecord`: ID, cwd, workspace/job order, typed target, captured - model, and non-optional start time. -- `TerminalReviewRecord`: ID, model, typed terminal, optional end time, summary, - and completed-only canonical result/parsed projection. -- `RestoredReviewRecord`: one compatible started + terminal pair. An active row - cannot be represented as a restored value. - -The live SQLite implementation lazily constructs and then explicitly owns -`any DatabaseWriter` and close state. Production constructs `DatabasePool` from -the exact URL during the first load; tests inject -`DatabaseQueue`/`DatabasePool`. Do not use `prepareDependencies`, -`@Dependency(\.defaultDatabase)`, or SQLiteData `defaultDatabase(...)`. -`CodexReviewStore` remains `@MainActor`; only immutable Sendable records cross the -boundary. The Store-owned mutation lane retains every task/receipt and shutdown -joins them before close. +| Record | Contents | +| --- | --- | +| `StartedReviewRecord` | ID, cwd, workspace and job order, typed target, captured model, non-optional start time | +| `TerminalReviewRecord` | ID, model, typed terminal, optional end time, summary, and completed-only final result and parsed findings | +| `RestoredReviewRecord` | A compatible start and terminal pair; active rows cannot form this value | -### Consumer path +The live adapter lazily creates and owns `any DatabaseWriter` and its close +state. Production opens a `DatabasePool` at the supplied URL on first load; +tests can inject a `DatabaseQueue` or `DatabasePool`. It does not use +`prepareDependencies`, `@Dependency(\.defaultDatabase)`, or SQLiteData +`defaultDatabase(...)`. `CodexReviewStore` remains `@MainActor`. -Before: +Consumers keep their existing construction: ```swift let store = CodexReviewStore.makeLiveStore(...) ReviewMonitorWindowController(store: store, ...) ``` -After: construction stays unchanged. App termination calls the additive public -`store.shutdown()` instead of runtime-only `stop()`. `CodexReviewHost` composes the -database behind `makeLiveStore`, and `ReviewUI` continues to observe the Store. - -## 5. Schema and invariants - -SQLiteData `@Table` records are storage models, not the domain aggregate. - -### `review_workspaces` - -- `cwd` primary key -- `sortOrder` - -### `review_records` - -- stable review ID primary key -- `cwd` foreign key to workspace -- application-wide, distinct manual `sortOrder`; repository-section views filter - this order without re-grouping rows by `cwd` -- typed target discriminator and variant payload -- captured/effective model -- lifecycle phase and typed terminal/cancellation/interruption fields -- `startedAt`, nullable `endedAt`, summary, canonical final review -- parsed-result state/source/parser version -- `terminalCommittedAt` and created/updated timestamps for deterministic retention - -### `review_findings` - -- stable finding ID primary key -- review ID foreign key with `ON DELETE CASCADE` -- ordinal, priority, title, body, path, start/end line -- unique `(reviewID, ordinal)` - -Schema rules: - -- `STRICT` tables, foreign keys, explicit indices, and versioned migrations. -- Published migrations are append-only; production never erases history on schema change. -- Active rows have no terminal payload. Terminal rows have one compatible typed terminal. -- Succeeded rows require non-empty canonical final review text no larger than the - existing 256 KiB domain limit. -- Findings and parsed-result metadata are one transaction with terminal commit. -- The persistence port does not carry session IDs, thread/turn IDs, exit code, - finding `rawText`, rendered projections, or raw log entries. -- Terminal mutation updates only terminal/result fields of an existing active row; - it cannot overwrite cwd, target, or manual order. -- A reorder renumbers the complete supplied repository-section scope in one Store - mutation. It never changes a review's `cwd` and never stores a second UI-only - ordering. -- New reviews reserve an application-wide order value. The schema migration from - workspace-local order first sorts by workspace order and then by the previous - review order before assigning unique application-wide values. -- Database load, start insertion, and ordering save reject duplicate review-order - values; a partial ordering update cannot collide with an omitted review. -- Restored rows derive display title, elapsed time, final flag, compact log entries, - and other projections; those values are not columns. +The host assembles storage behind `makeLiveStore`. App termination calls the +additive `store.shutdown()` instead of runtime-only `stop()`. + +## Schema + +SQLiteData `@Table` records describe storage rather than the observable job. + +`review_workspaces` stores `cwd` as its primary key and a `sortOrder`. + +`review_records` stores: + +- Stable review ID and workspace `cwd` foreign key +- Unique app-wide `sortOrder` +- Typed target and payload, captured/effective model +- Lifecycle and typed terminal, cancellation, and interruption fields +- `startedAt`, nullable `endedAt`, summary, and final review +- Parsed-result state, source, and parser version +- `terminalCommittedAt` and creation/update timestamps + +`review_findings` stores a stable finding ID, review foreign key with +`ON DELETE CASCADE`, ordinal, priority, title, body, path, and start/end line. +The pair `(reviewID, ordinal)` is unique. + +Tables are `STRICT`, with foreign keys, explicit indices, and versioned, +append-only migrations. Production keeps history during schema changes. +Active rows have no terminal payload. Terminal rows have a compatible typed +terminal; success requires non-empty final review text within the 256 KiB +limit. Findings and parser metadata commit with the terminal result. + +The port excludes session, thread, and turn IDs, exit code, raw logs, and +rendered values. Restored rows derive title, elapsed time, final flag, and +compact logs from stored fields. A terminal write changes only terminal and +result fields of an active row, preserving cwd, target, and order. + +Reordering updates the whole supplied repository section in one store mutation, +preserving each review's `cwd`. Views filter the app-wide order without +regrouping by workspace or storing another UI order. New reviews reserve unique +app-wide positions. Load, start insertion, and order saving reject duplicate +positions, including collisions with rows omitted from a partial update. ### Retention -- Keep at most 50 terminal reviews per workspace and 500 terminal reviews globally. -- Prune oldest terminal reviews by `(terminalCommittedAt, id)` after terminal commit - and at startup. -- The terminal transaction protects its current review ID from pruning so the - completing API can always read its result. -- Never prune the current nonterminal rows. -- Return pruned IDs from the transaction so Store membership matches durable membership. -- Remove workspace rows that no longer own active or terminal reviews. -- No time-based expiry in v1. - -## 6. Failure semantics - -- Open/migration/load/decode failure: publish history `.failed`, retain the database, - do not present empty history, continue runtime/auth/settings startup, reject new - review admission with an explicit I/O error. -- Start-header write failure: do not dispatch a backend review or publish a live row. -- Session/runtime/application close during start-header suspension: the start receipt - commits one typed requested-interruption terminal and dispatches no backend work. -- Terminal write failure: retain the current in-memory result, publish history - `.failed`, let the already completed review return its real outcome, and reject - subsequent starts until the next successful app launch. -- Delete/order failure: keep current durable membership/order, publish history - `.failed`, and do not silently claim success. -- Close failure: publish/log the failure and complete application termination only - after the close attempt returns. -- A queued/running row found at startup becomes - `.interrupted(.previousProcessExit)` with unknown `endedAt`; UI must not show a - running timer or invent an exact duration. -- Once public application shutdown enters `.closing`, `start` and `restart` cannot - admit a new runtime. Repeated shutdown callers join the same terminal completion. - -## 7. Variation points - -| Axis | Absorption point | Variant test | -| --- | --- | --- | -| live / preview / test persistence | composition injects `ReviewHistoryPersistence` | add one adapter and one factory registration | -| storage implementation | the history port | replace SQLite adapter without editing Store/UI | -| target kind | typed target codec in persistence | add one target case and one codec/schema migration | -| terminal cause | existing `ReviewTerminalRecord` mapping | add one typed cause mapping and round-trip test | -| runtime availability | sidebar/status presentation | history membership remains independent of server state | - -## 8. Deletions and avoided shapes - -### Deletions - -- Remove the unused `PreparedRecoveryEnvironment.withHistoryDatabaseURL` helper and - its isolated test; the live history location has one dedicated owner. -- Remove no Store/MCP public behavior. Restored rows use semantic reconstruction, - not a parallel UI-only history model. - -### Avoided shapes - -- Do not serialize `CodexReviewJob` or `ReviewLogEntry` wholesale. -- Do not call SQLiteData `@FetchAll` from `ReviewUI` leaf views. -- Do not add history operations to `CodexReviewStoreBackend`; runtime transport and - durable history are independent variation axes. -- Do not reuse `writeDiagnosticsIfNeeded` as persistence. It is optional test - diagnostics, catches write errors, and stores rendered/raw projections. -- Do not persist MCP session identity as future authorization. -- Do not use thread IDs as cross-process recovery tokens. -- Do not create an unowned database Task or rely on deinit for async close. -- Do not let the persistence executor serialize writes while MainActor applies their results - independently; both halves belong to the Store history-mutation lane. -- Do not fall back to a second database path or recreate a corrupt database. - -## 9. Test contract - -### Persistence adapter - -- fresh migration and schema constraints -- started/terminal round trip for every target and terminal variant -- findings transaction and cascade deletion -- startup orphan conversion with unknown end time -- per-workspace/global retention and returned pruned IDs -- invalid/incompatible row fails load without erasing data -- close rejects subsequent operations -- temporary-file and in-memory database configurations +Keep at most 50 terminal reviews per workspace and 500 globally. Prune the +oldest by `(terminalCommittedAt, id)` at startup and after terminal commit. +Protect the completing review in its transaction so the API can read its result, +and keep all active rows. Return pruned IDs to update store membership, then +remove workspaces with no reviews. Version 1 has no time-based expiry. + +## Failures + +| Failure | Result | +| --- | --- | +| Open, migration, load, or decode | Set history to `.failed`, retain the database, continue runtime/auth/settings startup, and reject new reviews with an I/O error | +| Start-header write | Publish no live row and dispatch no backend review | +| Session, runtime, or app closes during a start write | Commit one requested interruption and dispatch no backend work | +| Terminal write | Keep the in-memory outcome, set history to `.failed`, return the completed review's actual result, and reject new starts until a successful app launch | +| Delete or reorder | Keep durable membership/order, set history to `.failed`, and report failure | +| Close | Publish/log the error and wait for the close attempt before termination completes | + +On startup, queued and running rows become +`.interrupted(.previousProcessExit)` with unknown `endedAt`. The UI shows +neither a running timer nor an invented duration. Once application shutdown +enters `.closing`, start and restart cannot acquire a runtime; repeated shutdown +calls wait for the same completion. + +## Boundaries to keep + +Storage, target codecs, and terminal mappings can vary behind the history +interface. Live, preview, and test persistence are selected at composition. +A new target needs a codec/schema migration; a new terminal cause needs a typed +mapping and round-trip test. Runtime availability changes presentation without +changing history membership. + +Remove `PreparedRecoveryEnvironment.withHistoryDatabaseURL` and its isolated +test so the production history location has one owner. Keep the Store and MCP +public behavior, using restored semantic records in the existing UI model. + +Use the history port separately from `CodexReviewStoreBackend`; transport and +storage can vary independently. Keep SQLiteData `@FetchAll` out of leaf views. +`writeDiagnosticsIfNeeded` remains optional test output: it catches write +errors and includes raw/rendered values, so it cannot serve as persistence. +Retain database tasks and explicitly await close rather than relying on deinit. +A corrupt database reports failure without a fallback path or recreation. + +## Validation + +### Adapter + +Test fresh migration and constraints, every target and terminal round trip, +findings transactions and cascade deletion, abandoned-row conversion, retention, +and returned pruned IDs. Invalid or incompatible rows must fail without erasing +data, and operations after close must fail. Cover both temporary-file and +in-memory databases. ### Store -- history loads once before accepting review starts -- start header is durable before backend dispatch -- blocked start revalidates exact session/work admission/model/order receipt before dispatch -- start failure prevents dispatch and row publication -- terminal persistence completes before waiter, cancel response, runtime detach, and worker result finalization -- persistence failure is visible and blocks later starts without changing the real terminal outcome -- restored rows remain inaccessible to a new MCP session -- clean shutdown synchronizes terminal rows/order and closes history after workers -- shutdown is one-shot and rejects concurrent restart/runtime acquisition -- delete updates database, Store membership, workspace membership, and selection source -- overlapping reorder/delete/terminal prune preserves identical DB and Store order/membership -- reordering across primary-checkout/worktree rows in one repository section keeps - each review's cwd, preserves hidden filtered rows, and restores the same order - after persistence reload - -### ReviewUI / app - -- history remains visible for starting/stopped/failed server states -- restored success/failure/cancellation detail is non-empty -- terminal row with unknown end does not render a running timer -- history failure appears in the status presentation -- terminal context menu deletes; active context menu cancels -- every displayed insertion gap in one repository section accepts the same job - reorder contract, including gaps across workspace boundaries -- composition uses the production history path while preview/tests use injected stores -- application termination awaits `shutdown()` - -### Isolated application E2E - -- Rebuild `CodexReviewMonitor.app` from the branch. -- Launch that binary with a dedicated MCP port, diagnostics file, and temporary - history database path. Do not replace `HOME` or touch the user's production - history database. -- The E2E-only overrides are explicit environment/argument inputs owned by the - ReviewMonitor composition root: `REVIEW_MONITOR_TEST_PORT`, - `REVIEW_MONITOR_TEST_CODEX_COMMAND`, `REVIEW_MONITOR_TEST_DIAGNOSTICS_PATH`, and - `REVIEW_MONITOR_TEST_HISTORY_PATH`. They are not production fallback paths. -- Call its real Streamable HTTP MCP endpoint and complete a review against this - checkout. -- Terminate through `NSRunningApplication.terminate()` and wait for application - shutdown completion. -- Relaunch the same binary with the same isolated history path. -- Verify diagnostics and visible UI show exactly one restored terminal row with - target/model/status/final detail/findings and no command/reasoning transcript. -- Verify a newly initialized MCP session cannot list/read the restored row. -- Capture a screenshot of the restored sidebar/detail for the PR when the visible - change is reviewable. - -### Required gates +Verify that history loads once before admission and start headers save before +dispatch. After a suspended write, recheck the session, work admission, model, +and order. A failed start must publish no row and dispatch no backend work. + +Waiters, cancellation responses, runtime detach, and worker finalization must +wait for terminal persistence. A failed write must expose the error and block +later starts while preserving the completed review's outcome. Restored rows +must remain inaccessible to new MCP sessions. + +Test overlapping reorder, delete, and retention operations for matching database +and store membership/order. Deletion must also update workspace membership and +the selection source. Shutdown must drain workers, synchronize terminal rows +and order, wait for receipts, and close once, rejecting restart and runtime +acquisition during shutdown. + +### UI and app + +Verify history visibility during runtime startup, stop, and failure. Restored +success, failure, and cancellation details must be non-empty. Unknown end times +must have no running timer, and history errors must appear in status. Terminal +context menus delete; active context menus cancel. + +Reordering must accept every insertion gap in a repository section, including +between checkout and worktree rows. Keep each review's cwd and hidden filtered +rows, and restore the same order after reload. Verify production uses its history +path, previews/tests use injected stores, and app termination awaits shutdown. + +### Isolated app E2E + +Rebuild the app and launch it with a dedicated MCP port, diagnostics file, and +temporary database. The composition root owns these explicit inputs: +`REVIEW_MONITOR_TEST_PORT`, `REVIEW_MONITOR_TEST_CODEX_COMMAND`, +`REVIEW_MONITOR_TEST_DIAGNOSTICS_PATH`, and `REVIEW_MONITOR_TEST_HISTORY_PATH`. +Leave `HOME` and production history unchanged. + +Complete a real review through Streamable HTTP, terminate with +`NSRunningApplication.terminate()`, and wait for shutdown. Relaunch the same +binary with the same history path. Check diagnostics and visible UI for one +terminal row with target, model, status, final detail, and findings, without +command or reasoning transcripts. Verify a new MCP session cannot list or read +it. Capture the restored sidebar and detail for the PR. The executable procedure +is in the [E2E README](../scripts/review-history-e2e/README.md). ```bash swift test --build-system swiftbuild --no-parallel @@ -474,103 +357,47 @@ scripts/check-compatibility.sh git diff --check ``` -Then run branch-wide local Codex review against `main` until it reports no findings. +Then run branch-wide local Codex review against `main` until it has no findings. -## 10. Finding coverage +## Original delivery record -| Finding | Design response | -| --- | --- | -| F1 | One history port + SQLite owner + Store hydration | -| F2 | Storage records exclude job projection/runtime state | -| F3 | Started record stores the typed validated target | -| F4 | No generic log table; canonical result retains the existing hard bound | -| F5 | Restored compact semantic log projection | -| F6 | Sidebar membership independent of server availability | -| F7 | Restored non-live session identity and unchanged MCP filters | -| F8 | Dedicated Application Support owner; remove unused whole-environment helper | - -## 11. Migration slices and progress - -### Slice A — schema and adapter - -Budget: 6 hours; at most 10 production and 4 test files. - -- [x] Add SQLiteData dependency and internal target. -- [x] Add schema, migrations, codec, retention, close owner. -- [x] Pass focused persistence tests. - -### Slice B — Store cutover - -Budget: 8 hours; at most 12 production and 5 test files. - -- [x] Add semantic port/records and injected disabled/test implementations. -- [x] Load history, persist start/terminal, synchronize ordering, and delete. -- [x] Add application shutdown separate from runtime stop. -- [x] Pass focused Store/MCP tests. - -### Slice C — ReviewMonitor/UI composition - -Budget: 5 hours; at most 8 production and 4 test files. - -- [x] Compose production path/database. -- [x] Keep history visible during runtime failure. -- [x] Render history health and deletion semantics. -- [x] Pass focused ReviewUI/app tests. - -### Slice D — integration and delivery - -- [x] Run all repository gates. -- [x] Run branch-wide local Codex review to clean. -- [x] Commit final fixes and verify clean worktree. -- [ ] Push branch and create Ready PR to `main`. - -## 12. Acceptance remeasurement - -At completion, record: - -- final product/target graph from `swift package dump-package`; -- public/package/open distribution and any new public declarations; -- largest relevant files and `CodexReviewStore` stored-property change; -- remaining platform gates; -- the concrete path owner and DB close proof; -- old path/helper and alternate persistence routes removed; -- exact test/review results. - -Completion remeasurement before publication: - -- `swift package dump-package` confirms `CodexReviewPersistence` is internal and - depends only on `CodexReview` and `SQLiteData`; `CodexReviewHost` composes it; - `ReviewUI` has no persistence dependency. -- The only additive application-host surface is SPI - `ApplicationHostSupport`: one-shot `CodexReviewStore.shutdown()`, explicit - isolated-store factories, and the history-path test keys. The reviewed API - baseline and checksum include those additions; the original public live-store - factory is unchanged. -- Persistence behavior lives in `CodexReviewStoreHistory.swift` (760 lines) and - the internal adapter/codec/schema files (444/479/290 lines). Store adds the - availability, port, mutation-lane receipts, durable-ID sets, result leases, and - one-shot shutdown state; it does not add a second UI model or log cache. -- Production owns - `Application Support/CodexReviewMonitor/RecoveryV1/review-history.sqlite` via - the retained Application Support/application/recovery capability chain. App - termination cancels and joins launch, Store work, history receipts, database - close, and directory close in that order. Runtime restart does not close it. -- The unused whole-recovery history URL helper is removed. There is no alternate - persistence route, generic log table, raw transcript column, or SQLite import - in `ReviewUI`. -- `swift test --build-system swiftbuild --no-parallel`, the locked app test gate - (18 tests), all compatibility gates, schema/codec/retention tests, and the - actual-app semantic/UI E2E pass. The E2E rebuilt the app, ran Codex 0.149.1, - restored the same terminal job and `AccessGate.swift:3-3` finding after a clean - restart, verified MCP-session isolation, and captured accessibility text plus a - screenshot. -- Branch-wide local Codex review against `main` completed with zero findings. -- The repo-standard app command without flags is blocked before compilation by - local Xcode macro trust. CI/release/E2E use the committed workspace lock with - automatic resolution disabled and `-skipMacroValidation`; the same app tests - pass through that non-interactive path. -- Runtime shutdown closes MCP admission first and drains every admitted finite - JSON-RPC response through the HTTP response-end acknowledgement before it - disconnects semantic sessions or shuts down the event-loop group. The E2E - therefore requires curl status 0 and a complete JSON-RPC/SSE response; durable - history remains recovery evidence rather than a fallback transport result. +| Slice | Planned budget | Recorded completion | +| --- | --- | --- | +| A: schema and adapter | 6 hours; at most 10 production and 4 test files | Dependency, internal target, schema, migrations, codec, retention, close owner, and focused tests complete | +| B: store | 8 hours; at most 12 production and 5 test files | Port and injected disabled/test adapters; load, start/terminal writes, order, delete, shutdown, and Store/MCP tests complete | +| C: app and UI | 5 hours; at most 8 production and 4 test files | Production path/database, history visibility/health/deletion, and UI/app tests complete | +| D: delivery | — | Repository checks, clean local review, commits, and clean worktree complete; push and Ready PR still unchecked | + +Completion measurements were intended to cover the product/target graph, +public/package/open declarations, largest files and store properties, remaining +platform gates, location and close ownership, deleted alternate routes, and +exact validation results. The recorded results were: + +- `swift package dump-package` kept persistence internal, depending on + `CodexReview` and SQLiteData. Host assembled it; UI had no storage dependency. +- Added ApplicationHostSupport SPI covered one-shot `shutdown()`, isolated-store + factories, and history test keys. API baseline and checksum included these + additions; the public live-store factory stayed unchanged. +- History store code had 760 lines; adapter, codec, and schema had 444, 479, and + 290. The store added availability, port, mutation receipts, durable-ID sets, + result leases, and shutdown state, without another UI model or log cache. +- The production location was + `Application Support/CodexReviewMonitor/RecoveryV1/review-history.sqlite`, + retained through Application Support/application/recovery capabilities. + Termination cancelled and waited for launch, store work, history receipts, + database close, and directory close in that order. Runtime restart kept it open. +- The unused helper was removed, with no alternate database route, generic log + table, transcript column, or SQLite import in UI. +- Package tests, the locked app test run (18 tests), compatibility checks, + schema/codec/retention tests, and actual-app semantic/UI E2E passed. E2E used + Codex 0.149.1 and restored the same terminal job and `AccessGate.swift:3-3` + finding after restart. It checked MCP isolation and captured accessibility + text and a screenshot. Local branch review reported zero findings. +- The standard app test command was blocked before compilation by local Xcode + macro trust. CI, release, and E2E used the committed workspace lock, + disabled automatic resolution, and `-skipMacroValidation`; app tests passed + through that path. +- Runtime shutdown closed MCP admission, drained finite JSON-RPC responses + through HTTP response-end acknowledgement, then disconnected sessions and + closed the event-loop group. E2E required curl status 0 and a complete + JSON-RPC/SSE response; restored history could not substitute for that response. diff --git a/README.md b/README.md index 8fab7c51..46d49eaa 100644 --- a/README.md +++ b/README.md @@ -1,20 +1,18 @@ # CodexReviewKit -CodexReviewKit is the native macOS companion app for Codex review. +CodexReviewMonitor is a macOS app for running Codex reviews. Start reviews from +Codex or Claude Code through MCP, then read the output and findings in the app. +It requires macOS 26 or newer and the Codex CLI installed on your Mac. -Launch `CodexReviewMonitor.app`, register its MCP endpoint with Codex, then run -reviews through the `codex_review` tools while the app keeps the review state -visible. - -## Quick Start +## Quick start 1. Download the signed and notarized DMG from the [latest release](https://github.com/lynnswap/CodexReviewKit/releases/latest). - 2. Open `CodexReviewMonitor_.dmg`, drag `CodexReviewMonitor.app` to - Applications, then launch the app. - -3. Register the local MCP endpoint in the client you use. + Applications, and launch it. +3. Sign in from the app with **Sign in with ChatGPT**, or choose + **Sign in another way** to use an API key. +4. Register the app's MCP endpoint with your client. Codex CLI: @@ -28,62 +26,18 @@ visible. claude mcp add --transport http codex_review http://localhost:9417/mcp ``` -4. Use the review tools from Codex: - - - `review_start` - - `review_await` - - `review_list` - - `review_read` - - `review_cancel` - -## What Runs Locally - -- `CodexReviewMonitor.app` shows review jobs, output, and findings. -- `http://localhost:9417/mcp` is the app-managed MCP endpoint. -- `codex app-server` runs behind CodexReviewMonitor as the live review backend. -- `~/.codex_review` is the dedicated Codex home used by CodexReviewMonitor. - -## Codex Updates - -ReviewMonitor checks the selected Codex installation at launch and every eight -hours. Use **Settings → Updates → Check for Updates** for a manual check and its -last-check time. Manual checks do not change the automatic schedule and do not -install an update. Unsupported installations and failed checks are reported -separately from **Up to Date**. Automatic installation supports the stable -Homebrew Codex cask and standalone installations selected through their stable -launcher. Update checks and installation use the selected CLI's installation -home, including custom standalone homes; reviews keep using `~/.codex_review`. -When a newer version is available for an installation that cannot be updated -automatically, Settings reports **Update Available** with manual update guidance. - -When an update is available, choose **Update** in the sidebar toolbar. During a -review, **Update After Reviews** lets current reviews finish and queues new -requests inside ReviewMonitor. **Stop Reviews and Update** cancels current -reviews and updates immediately. ReviewMonitor and its MCP sessions stay open; -queued requests resume after Codex restarts, without being resubmitted. -The toolbar shows a spinner and the update stage while Codex is stopping, -installing, or restarting. - -If updating fails but Codex can restart, queued reviews resume and the error -remains visible in Settings. If Codex cannot restart, the queue is retained and -**Retry** in the sidebar attempts runtime recovery without reinstalling. Explicitly -quitting the app cancels queued reviews; an installation already in progress is -allowed to finish before the app exits. - -To investigate UI responsiveness during updates, enable -`REVIEW_MONITOR_SIMULATE_CODEX_UPDATE=1` in the Xcode scheme's **Run → Arguments → -Environment Variables**, then launch the app. The normal **Update** button appears -after a one-second simulated check. Installation waits ten seconds using the same -subprocess runner as a real update, while the existing Codex runtime stops and -restarts normally. Codex and Homebrew packages are not changed. Settings reports -**Up to Date** afterward; relaunch the app to repeat the simulation. Use this mode -with the live runtime, with `REVIEW_MONITOR_MOCK_JOBS` and -`REVIEW_MONITOR_REVIEW_MODE` disabled. - -## Timeout Setup - -Long reviews can exceed the default MCP client timeout. `codex mcp add` does -not currently expose timeout flags, so add them manually after registration: +5. Ask your client to review a repository. It uses `review_start` to start a job + and `review_await` if the review needs more time. `review_list`, `review_read`, + and `review_cancel` let you inspect or cancel jobs. + +Keep the app running while you use the tools. It hosts +`http://localhost:9417/mcp` and runs `codex app-server` for the reviews. +ReviewMonitor uses `~/.codex_review` as its Codex home. + +### Allow time for long reviews + +Add these timeout settings to the calling Codex client's configuration after +registering the endpoint. `codex mcp add` does not expose timeout flags. ```toml [mcp_servers.codex_review] @@ -92,50 +46,55 @@ startup_timeout_sec = 1200.0 tool_timeout_sec = 1200.0 ``` -This config belongs to the Codex client that calls the MCP server. It is -separate from CodexReviewMonitor's dedicated runtime home at `~/.codex_review`. +This is the client's configuration, separate from ReviewMonitor's +`~/.codex_review` home. See the [MCP reference](Docs/mcp.md) for tool arguments, +results, and session behavior. -## Build from Source +## Update Codex -To build a local DMG from the current checkout, run from the repository root -using Python 3.10 or newer: +ReviewMonitor checks for Codex updates at launch and every eight hours. You can +also check in **Settings → Updates → Check for Updates**, which shows the last +check time. A manual check only checks availability; it keeps the automatic +schedule and leaves installation to you. +Settings distinguishes unsupported installations and failed checks from +**Up to Date**. -```bash -python3 scripts/build_review_monitor.py -``` +Choose **Update** in the sidebar when a new version is available. If reviews are +running, choose **Update After Reviews** to finish them first, or +**Stop Reviews and Update** to cancel them and update now. New review requests +wait in ReviewMonitor's queue and resume after Codex restarts. The app and MCP +sessions stay open, and you do not need to resubmit requests. -The command creates `dist/CodexReviewMonitor_yymmdd_hhmm.dmg`, using the local -build-start time. It applies an ad-hoc hardened-runtime signature to the app and -verifies the DMG's contents. Build caches remain -in `.build` for subsequent builds. The first run prepares the pinned DMG tools -in `.build/release-tools` and may download the locked package dependencies. +Automatic installation supports the stable Homebrew Codex cask and standalone +installations selected through their stable launcher. Other installations show +manual update guidance when an update is available. Checks and installation use +the selected CLI's installation home, including custom standalone homes; +reviews use `~/.codex_review`. -Keep CodexReviewMonitor running while building. When the DMG is ready and reviews -have finished, quit the app, open the DMG, and drag `CodexReviewMonitor.app` to -Applications. Choose **Replace**, then launch the installed app. There is no need -to delete the existing app first. Builds started in the same minute replace the -same DMG only after the new image passes validation; older filenames are retained. +If installation fails but Codex restarts, queued reviews resume and Settings +shows the error. If Codex cannot restart, the queue stays available and **Retry** +attempts recovery without reinstalling. Quitting the app cancels queued reviews +and waits for any installation already in progress. -The local build requires an Apple silicon Mac and Xcode 26.4 or newer; the app -requires macOS 26 or newer. The command does not modify or launch installed apps. +## Build from source -The default ad-hoc signature is for local use and does not make a redistributable -or notarized app. If the Mac's management policy requires an approved local -identity, pass it explicitly; the command never falls back to another -identity: +On an Apple silicon Mac with Xcode 26.4 or newer and Python 3.10 or newer, run +this command from the repository root: ```bash -python3 scripts/build_review_monitor.py \ - --signing-identity 'Apple Development: Developer Name (TEAMID)' +python3 scripts/build_review_monitor.py ``` -Device-management policy can still prohibit locally signed apps. The command -does not disable Gatekeeper or remove quarantine metadata. +It creates `dist/CodexReviewMonitor_yymmdd_hhmm.dmg` with an ad-hoc signature for +local use. You can keep ReviewMonitor running during the build. Once reviews +finish, quit the app, open the DMG, and drag the new app to Applications. Choose +**Replace**, then launch it. + +See [local build details](Docs/releases.md#local-builds) for signing identities, +build caches, and packaging behavior. -## More Detail +## Documentation -- [Architecture](Docs/architecture.md): ownership boundaries and runtime flow. -- [MCP reference](Docs/mcp.md): tool schemas, discovery resources, session - behavior, and runtime files. -- [Release guide](Docs/releases.md): maintainer signing setup, Draft Release - preparation and publication, validation builds, and repository protection. +- [Architecture](Docs/architecture.md): targets, review flow, and runtime updates. +- [MCP reference](Docs/mcp.md): tool arguments, results, and runtime files. +- [Release guide](Docs/releases.md): signing, validation, and publication. diff --git a/Sources/CodexReview/Model/CodexReviewJob.swift b/Sources/CodexReview/Model/CodexReviewJob.swift index 056e0a87..a296d810 100644 --- a/Sources/CodexReview/Model/CodexReviewJob.swift +++ b/Sources/CodexReview/Model/CodexReviewJob.swift @@ -454,9 +454,9 @@ public final class CodexReviewJob: Identifiable, Hashable { package let acceptedAt: Date package let origin: CodexReviewJobOrigin package let target: CodexReviewAPI.Target - /// A distinct application-wide manual-order position. Higher values sort first. + /// This review's unique position in the app's manual order. Higher values sort first. /// - /// The value may be renormalized after a reorder; use `id` for stable identity. + /// Reordering can change this value; use `id` to identify the review. public internal(set) var sortOrder: Double public internal(set) var targetSummary: String public internal(set) var core: ReviewJobCore diff --git a/Sources/CodexReview/ReviewRecoveryContract.swift b/Sources/CodexReview/ReviewRecoveryContract.swift index 0d75a3af..5d4715bb 100644 --- a/Sources/CodexReview/ReviewRecoveryContract.swift +++ b/Sources/CodexReview/ReviewRecoveryContract.swift @@ -27,9 +27,8 @@ package enum ReviewAttemptRecoveryTrigger: Equatable, Sendable { } } -/// Exhaustively classifies why an admitted review event stream ended without a -/// product terminal. Recovery policy is derived from this value once, at the -/// attempt admission boundary. +/// The reason a review's event stream ended without a final result. +/// The attempt admission uses this reason once to decide whether recovery is allowed. package enum ReviewAttemptStreamFailure: LocalizedError, Equatable, Sendable { case recoverableNetwork(ReviewRuntimeCloseFailure) case modelCapacity(message: String?) @@ -105,8 +104,8 @@ package struct ReviewRecoveryCandidateAlreadyPrepared: LocalizedError, Equatable } } -/// Copies share one preparation owner. Exactly one caller can turn a resolved -/// attempt into a handoff, even when preparation is requested concurrently. +/// A resolved attempt that can prepare one recovery handoff. +/// Copies share the preparation state, so concurrent callers cannot prepare it twice. package struct ReviewRecoveryCandidate: Equatable, Sendable { package let resolved: ReviewResolvedAttemptTerminal package let trigger: ReviewAttemptRecoveryTrigger @@ -197,8 +196,8 @@ package enum ReviewRecoveryStagingFailure: LocalizedError, Equatable, Sendable { } } -/// Copies share one consumption owner, so a resume token can be consumed or -/// discarded exactly once even when concurrent callers retain the value. +/// A recovery token that can be consumed or discarded once. +/// Copies share that state, including when used by concurrent callers. package struct ReviewRecoveryHandoff: Equatable, Sendable { package struct Consumption: Equatable, Sendable { package let candidate: ReviewRecoveryCandidate @@ -262,8 +261,8 @@ private actor ReviewRecoveryHandoffConsumptionOwner { } } -/// Identifies the exact source attempt and runtime generation owned by one -/// backend recovery route. Mutable route state remains backend-owned. +/// Identifies the source attempt and runtime generation for a backend recovery route. +/// The backend keeps the route's mutable state. package final class ReviewRecoveryRouteReceipt: Sendable { package let sourceRun: CodexReviewBackendModel.Review.Run package let sourceGeneration: ReviewRuntimeGeneration diff --git a/Sources/CodexReview/ReviewStartAdmission.swift b/Sources/CodexReview/ReviewStartAdmission.swift index a71837c2..b7476256 100644 --- a/Sources/CodexReview/ReviewStartAdmission.swift +++ b/Sources/CodexReview/ReviewStartAdmission.swift @@ -176,9 +176,9 @@ package struct ReviewStartAdmissionContractFailure: LocalizedError, Equatable, S } } -/// Owns the request admissions that create and interrupt one backend review run. -/// A dispatched request stays outcome-unknown until its response, an explicit -/// rejection, or a typed terminal resolves it. +/// Tracks requests that start or interrupt one backend review run. +/// After dispatch, a request's outcome stays unknown until a response, explicit +/// rejection, or terminal event resolves it. package actor ReviewStartAdmission { package struct CancellationRequestRegistration: Equatable, Sendable { package enum Disposition: Equatable, Sendable { @@ -238,8 +238,8 @@ package actor ReviewStartAdmission { preparedRun: CodexReviewBackendModel.Review.Run, dispatch: RequestDispatch ) - /// The source thread may have been loaded and subscribed by `thread/resume`. - /// Retain its cleanup owner until the response is acknowledged. + /// `thread/resume` may already have loaded and subscribed to the source thread. + /// Keep responsibility for its cleanup until the response is acknowledged. case resumingRecovery( predecessorRun: CodexReviewBackendModel.Review.Run ) @@ -420,9 +420,8 @@ package actor ReviewStartAdmission { } } - /// Installs an already-existing source thread for a replacement attempt. - /// Unlike `recordPreparedThread`, this transition proves that no new - /// `thread/start` request is part of the replacement. + /// Prepares a replacement attempt on an existing source thread. + /// Use this when recovery creates no new thread with `thread/start`. package func recordPreparedRecoveryRun( _ run: CodexReviewBackendModel.Review.Run ) throws { diff --git a/Sources/CodexReview/Store/CodexReviewStoreUpdate.swift b/Sources/CodexReview/Store/CodexReviewStoreUpdate.swift index a94c2ced..480fa341 100644 --- a/Sources/CodexReview/Store/CodexReviewStoreUpdate.swift +++ b/Sources/CodexReview/Store/CodexReviewStoreUpdate.swift @@ -15,9 +15,10 @@ extension CodexReviewStore { case failed(String) } - /// Keeps MCP sessions and accepted jobs alive while replacing the Codex runtime. - /// Concurrent callers join the existing operation; only its installation closure runs. - /// An installation failure is thrown even when the runtime recovers and jobs resume. + /// Updates Codex and restarts its runtime, preserving MCP sessions and accepted jobs. + /// + /// Concurrent calls wait for the same update, using the first call's installation + /// closure. Throws installation errors even if the runtime restarts and jobs resume. @_spi(ApplicationHostSupport) public func updateCodex( when timing: CodexUpdateTiming, install: @escaping @MainActor @Sendable () async throws -> Void diff --git a/Sources/CodexReview/Store/StoreReviewRecoveryReceipt.swift b/Sources/CodexReview/Store/StoreReviewRecoveryReceipt.swift index 09c371fd..84ac439d 100644 --- a/Sources/CodexReview/Store/StoreReviewRecoveryReceipt.swift +++ b/Sources/CodexReview/Store/StoreReviewRecoveryReceipt.swift @@ -20,9 +20,9 @@ package struct StoreReviewActiveAttempt: Sendable { } } -/// Store must retain this receipt through a joined suppression or committed -/// promotion. `isolated deinit` cancels only as a synchronous misuse backstop; -/// it cannot join work or discard an exact backend resource. +/// Keeps recovery work alive until the store finishes suppressing or committing it. +/// The store must retain this receipt and wait for that work. `isolated deinit` +/// only cancels tasks; it cannot wait for them or discard backend resources. @MainActor package final class StoreReviewRecoveryReceipt { package enum Completion { diff --git a/Sources/CodexReviewHost/CodexExecutableResolver.swift b/Sources/CodexReviewHost/CodexExecutableResolver.swift index 4531931f..794a672c 100644 --- a/Sources/CodexReviewHost/CodexExecutableResolver.swift +++ b/Sources/CodexReviewHost/CodexExecutableResolver.swift @@ -171,7 +171,7 @@ package struct CodexExecutableResolver: Sendable { } public extension CodexReviewRuntime { - /// Selects the CLI using the review runtime's precedence, preserving the launcher for updates. + /// Selects the review runtime's CLI and keeps its launcher path for updates. @_spi(ApplicationHostSupport) static func resolveExecutable( configuredPath: String?, environment: [String: String] diff --git a/Sources/CodexReviewHost/CodexReviewNativeAuthentication.swift b/Sources/CodexReviewHost/CodexReviewNativeAuthentication.swift index bf5028d6..ad8d0f6a 100644 --- a/Sources/CodexReviewHost/CodexReviewNativeAuthentication.swift +++ b/Sources/CodexReviewHost/CodexReviewNativeAuthentication.swift @@ -11,7 +11,7 @@ public enum CodexReviewNativeAuthentication {} public extension CodexReviewNativeAuthentication { struct Configuration: Sendable { public enum BrowserSessionPolicy: Sendable { - /// Allows the authentication session to share the browser's cookies and browsing data. + /// Uses a browser session that can share cookies and browsing data. case shared case ephemeral } diff --git a/Sources/CodexReviewHost/Filesystem/DirectoryCapability.swift b/Sources/CodexReviewHost/Filesystem/DirectoryCapability.swift index 06d59b71..be522521 100644 --- a/Sources/CodexReviewHost/Filesystem/DirectoryCapability.swift +++ b/Sources/CodexReviewHost/Filesystem/DirectoryCapability.swift @@ -324,7 +324,8 @@ package final class DirectoryCapability: Sendable { ) } - /// Returns `nil` only when absolute descriptor acquisition reports `ENOENT`. + /// Opens the directory, returning `nil` only if opening its absolute path reports `ENOENT`. + /// Validation failures and other errors are thrown. package static func openExistingIfPresent( at absoluteURL: URL, requirements: Requirements @@ -459,7 +460,8 @@ package final class DirectoryCapability: Sendable { } } - /// Creates an empty `0600` file, or validates and preserves an existing regular file unchanged. + /// Creates an empty file with mode `0600` if it is missing. + /// Validates an existing regular file without changing it. package func createFileIfMissing(named name: Name) throws { try withBorrowedDescriptor { parent in try Self.validateOwned(parent, capability: self) @@ -608,13 +610,13 @@ package final class DirectoryCapability: Sendable { } } - /// Recursively removes a directory after descriptor-relative identity validation, without - /// following symbolic links. A root missing at initial inspection is a no-op; observable identity - /// changes, mount boundaries, and unsupported entries fail after any completed removals. + /// Removes a directory tree after checking identities relative to the parent descriptor. + /// Symbolic links are not followed. A root missing at the first check is a no-op. + /// Identity changes, mount boundaries, or unsupported entries throw an error; + /// removals already completed are not rolled back. /// - /// This operation does not provide identity-conditional unlink against a malicious same-UID - /// process that renames an entry in the final `fstatat`-to-`unlinkat` window. Callers requiring - /// that stronger guarantee must not use this API. + /// A process with the same UID can still replace an entry between the final `fstatat` + /// and `unlinkat`. Use this only where malicious replacements are outside the threat model. package func removeDirectoryRecursively( named name: Name, expectedIdentity: Identity @@ -632,12 +634,11 @@ package final class DirectoryCapability: Sendable { } } - /// Removes a regular-file entry after descriptor-relative identity and type revalidation. - /// Descriptor-relative `ENOENT` is a no-op. + /// Removes a regular file after rechecking its identity and type relative to the parent descriptor. + /// A missing entry (`ENOENT`) is a no-op. /// - /// This operation does not provide identity-conditional unlink against a malicious same-UID - /// process that renames an entry in the final `fstatat`-to-`unlinkat` window. Callers requiring - /// that stronger guarantee must not use this API. + /// A process with the same UID can still replace an entry between the final `fstatat` + /// and `unlinkat`. Use this only where malicious replacements are outside the threat model. package func removeFile(named name: Name) throws { try withBorrowedDescriptor { parent in try Self.validateOwned(parent, capability: self) diff --git a/Tools/ReviewMonitor/CodexReviewMonitor/ReviewMonitorCodexUpdater.swift b/Tools/ReviewMonitor/CodexReviewMonitor/ReviewMonitorCodexUpdater.swift index c1743624..d0ae2405 100644 --- a/Tools/ReviewMonitor/CodexReviewMonitor/ReviewMonitorCodexUpdater.swift +++ b/Tools/ReviewMonitor/CodexReviewMonitor/ReviewMonitorCodexUpdater.swift @@ -78,7 +78,7 @@ final class ReviewMonitorCodexUpdater { } } - /// Stop read-only checking before Store shutdown cancels a deferred update or joins installation. + /// Stops update checks before store shutdown cancels a deferred update or waits for installation. func stopChecking() async { stopping = true monitorTask?.cancel() diff --git a/scripts/review-history-e2e/README.md b/scripts/review-history-e2e/README.md index 5a08a8aa..4de4605a 100644 --- a/scripts/review-history-e2e/README.md +++ b/scripts/review-history-e2e/README.md @@ -1,53 +1,60 @@ -# Review history application E2E +# Test review history across an app restart -`run.sh` is the isolated macOS semantic gate for durable ReviewMonitor history. It builds -the app into a dedicated DerivedData directory, runs an actual review through -`/opt/homebrew/bin/codex`, gracefully quits the exact app PID, relaunches against -the same SQLite database, and checks restored Store diagnostics plus MCP session -isolation. Complete app acceptance also requires the visible UI/accessibility -inspection and screenshot described below; diagnostics do not prove that the -sidebar or detail renderer is correct. +`run.sh` builds ReviewMonitor, runs a real Codex review, quits that app instance, +and relaunches it with the same SQLite database. It checks that the store restores +the result and that a new MCP session cannot access the restored job. A full +acceptance run also includes the UI inspection below. -The application composition root must implement all four explicit test inputs: +## Before running + +Authenticate `/opt/homebrew/bin/codex` in ReviewMonitor's Codex home, normally +`~/.codex_review`. The script uses that login and creates a temporary Git fixture +with an unsafe uncommitted change to produce a finding. + +The app's composition root must support these isolated test inputs: - `REVIEW_MONITOR_TEST_PORT` - `REVIEW_MONITOR_TEST_CODEX_COMMAND` - `REVIEW_MONITOR_TEST_DIAGNOSTICS_PATH` - `REVIEW_MONITOR_TEST_HISTORY_PATH` -The script fails when that integration is absent. It never changes `HOME`, never -uses port `9417`, and never falls back to the production history location. The -fixture is a new temporary Git repository with one intentionally unsafe -uncommitted change so the real review produces a structured finding. The -effective ReviewMonitor Codex home (default `~/.codex_review`) must already be -authenticated for `/opt/homebrew/bin/codex`; the gate does not perform login or -redirect `HOME`. +The script fails if those inputs are unavailable. It uses a dedicated +DerivedData directory, port, and history path. It leaves `HOME`, port `9417`, +and the production history database unchanged. -Run the gate from the repository root: +## Run the automated checks + +From the repository root: ```bash scripts/review-history-e2e/run.sh ``` -Every run retains its artifact directory, including build/app logs, MCP requests -and responses, semantic diagnostics, SQLite schema/rows, and a final summary. A -failure prints that directory and gracefully terminates only the exact app PID it -started; a verified process that ignores graceful termination is checked again by -executable path before an exact signal fallback. +Each run keeps an artifact directory with build and app logs, MCP requests and +responses, store diagnostics, SQLite schema and rows, and `e2e-summary.json`. + +On failure, the script prints that directory and terminates the exact app PID +it started. It first requests a graceful quit. If the process stays alive, it +checks the executable path again before sending a signal. -For the required final visible UI inspection, leave the verified second instance running: +## Inspect the restored UI + +Run with the second app instance left open: ```bash scripts/review-history-e2e/run.sh --keep-restored-app-running ``` -The output and `e2e-summary.json` identify the restored app PID, rebuilt binary, -diagnostics, database, fixture, and job. Inspect the rebuilt process through the -macOS accessibility tree, select the restored row, and verify its target, -terminal state, duration, canonical review, and `AccessGate.swift` finding. Save -a screenshot as `ui-restored.png` in the artifact directory, record the inspected -accessibility state beside it, and change `uiEvidenceStatus` from `pending` only -after both checks pass. Finally, run the exact termination command printed by the -script. The script intentionally remains attached until that exact app process -terminates, so a non-interactive runner can inspect the UI without the child being -re-launched outside the isolated environment. +The output and `e2e-summary.json` identify the app PID, rebuilt binary, +diagnostics, database, fixture, and job. Use that process's accessibility tree to +select the restored row. Check its target, terminal state, duration, final +review, and `AccessGate.swift` finding in the sidebar and detail view. + +Save `ui-restored.png` in the artifact directory and record the inspected +accessibility state beside it. Set `uiEvidenceStatus` from `pending` only after +both the screenshot and accessibility checks pass; store diagnostics alone do +not verify rendering. + +Finish with the exact termination command printed by the script. The script +waits for that app process to exit, keeping it in the isolated environment +throughout the inspection.