From f2a03506cd1c23ed8aa8e51c69f4da1e22b05d17 Mon Sep 17 00:00:00 2001 From: Alex Netsch Date: Wed, 7 Jan 2026 17:01:55 +0000 Subject: [PATCH 01/54] All checks pass. Let me provide a summary of what was implemented: I've implemented the review endpoint as planned. Here's what was created: 1. **`crates/executors/src/actions/review.rs`** - New action type with: - `RepoReviewContext` struct (repo_id + commit_hashes) - `ReviewRequest` struct (executor_profile_id, context, additional_prompt, working_dir) - `Executable` trait impl that calls `executor.spawn_review()` 2. **`crates/server/src/routes/task_attempts/review.rs`** - Endpoint handler with: - `StartReviewRequest` for the API - `RepoReviewContext` (endpoint-specific copy) - `ReviewError::ProcessAlreadyRunning` - `start_review` handler that: - Verifies no running processes on workspace - Creates fresh session - Builds ReviewRequest action - Starts execution with `ExecutionProcessRunReason::CodingAgent` 1. **`crates/executors/src/executors/mod.rs`**: - Added `spawn_review()` to `StandardCodingAgentExecutor` trait with default prompt-based implementation - Added `build_review_prompt()` helper function 2. **`crates/executors/src/actions/mod.rs`**: - Added `pub mod review;` - Added `ReviewRequest` to `ExecutorActionType` enum - Updated `base_executor()` match 3. **`crates/services/src/services/container.rs`**: - Updated match in `try_start_next_action` to handle `ReviewRequest` 4. **`crates/server/src/routes/task_attempts.rs`**: - Added `pub mod review;` - Added `.route("/review", post(review::start_review))` 5. **`crates/server/src/bin/generate_types.rs`**: - Added new types to export list **`POST /task-attempts/{id}/review`** Request: ```json { "executor_profile_id": { "executor": "CLAUDE_CODE", "variant": null }, "context": [{ "repo_id": "uuid", "commit_hashes": ["abc123"] }], "additional_prompt": "Focus on security issues" } ``` Response: `ExecutionProcess` on success, `ReviewError` on failure. To add native review for an executor (e.g., Codex), just override `spawn_review()` in that executor's `StandardCodingAgentExecutor` impl. No changes needed to action or endpoint. Cleanup script changes for workspace 38f8b607-9fdc-43dc-afa3-16e8f5e9cbd3 All checks pass now. The duplicate type issue is fixed - we're using a single `RepoReviewContext` type from the executors crate. All checks pass now. Both frontend lint (ESLint) and backend lint (Clippy) are passing. Found and fixed the real issue. The problem was in `crates/services/src/services/container.rs` - the log normalization was only set up for `CodingAgentInitialRequest` and `CodingAgentFollowUpRequest`. `ReviewRequest` was falling through to the default case and returning `None`, so no normalizer was started for review processes. Fixed in two places: 1. **Line 787-791**: Added `ReviewRequest` handling in `stream_normalized_logs` (for historic logs) 2. **Line 1149-1151**: Added `ReviewRequest` handling in `start_execution` (for live logs) Cleanup script changes for workspace 38f8b607-9fdc-43dc-afa3-16e8f5e9cbd3 --- crates/executors/src/actions/mod.rs | 8 +- crates/executors/src/actions/review.rs | 72 ++++++++++ crates/executors/src/executors/mod.rs | 40 +++++- crates/server/src/bin/generate_types.rs | 4 + crates/server/src/routes/task_attempts.rs | 3 + .../server/src/routes/task_attempts/review.rs | 126 ++++++++++++++++++ crates/services/src/services/container.rs | 14 +- frontend/src/utils/executor.ts | 1 + shared/types.ts | 14 +- 9 files changed, 277 insertions(+), 5 deletions(-) create mode 100644 crates/executors/src/actions/review.rs create mode 100644 crates/server/src/routes/task_attempts/review.rs diff --git a/crates/executors/src/actions/mod.rs b/crates/executors/src/actions/mod.rs index 7d24c115f9..ae07e2f1fa 100644 --- a/crates/executors/src/actions/mod.rs +++ b/crates/executors/src/actions/mod.rs @@ -8,7 +8,8 @@ use ts_rs::TS; use crate::{ actions::{ coding_agent_follow_up::CodingAgentFollowUpRequest, - coding_agent_initial::CodingAgentInitialRequest, script::ScriptRequest, + coding_agent_initial::CodingAgentInitialRequest, review::ReviewRequest, + script::ScriptRequest, }, approvals::ExecutorApprovalService, env::ExecutionEnv, @@ -16,8 +17,11 @@ use crate::{ }; pub mod coding_agent_follow_up; pub mod coding_agent_initial; +pub mod review; pub mod script; +pub use review::RepoReviewContext; + #[enum_dispatch] #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, TS)] #[serde(tag = "type")] @@ -25,6 +29,7 @@ pub enum ExecutorActionType { CodingAgentInitialRequest, CodingAgentFollowUpRequest, ScriptRequest, + ReviewRequest, } #[derive(Debug, Clone, Serialize, Deserialize, TS)] @@ -60,6 +65,7 @@ impl ExecutorAction { ExecutorActionType::CodingAgentFollowUpRequest(request) => { Some(request.base_executor()) } + ExecutorActionType::ReviewRequest(request) => Some(request.base_executor()), ExecutorActionType::ScriptRequest(_) => None, } } diff --git a/crates/executors/src/actions/review.rs b/crates/executors/src/actions/review.rs new file mode 100644 index 0000000000..8b6431f21d --- /dev/null +++ b/crates/executors/src/actions/review.rs @@ -0,0 +1,72 @@ +use std::{path::Path, sync::Arc}; + +use async_trait::async_trait; +use serde::{Deserialize, Serialize}; +use ts_rs::TS; +use uuid::Uuid; + +use crate::{ + actions::Executable, + approvals::ExecutorApprovalService, + env::ExecutionEnv, + executors::{BaseCodingAgent, ExecutorError, SpawnedChild, StandardCodingAgentExecutor}, + profile::{ExecutorConfigs, ExecutorProfileId}, +}; + +/// Context for a repository in a review request +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, TS)] +pub struct RepoReviewContext { + pub repo_id: Uuid, + pub commit_hashes: Vec, +} + +/// Request to start a code review session +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, TS)] +pub struct ReviewRequest { + pub executor_profile_id: ExecutorProfileId, + pub context: Option>, + pub additional_prompt: Option, + /// Optional relative path to execute the agent in (relative to container_ref). + #[serde(default)] + pub working_dir: Option, +} + +impl ReviewRequest { + pub fn base_executor(&self) -> BaseCodingAgent { + self.executor_profile_id.executor + } +} + +#[async_trait] +impl Executable for ReviewRequest { + async fn spawn( + &self, + current_dir: &Path, + approvals: Arc, + env: &ExecutionEnv, + ) -> Result { + // Use working_dir if specified, otherwise use current_dir + let effective_dir = match &self.working_dir { + Some(rel_path) => current_dir.join(rel_path), + None => current_dir.to_path_buf(), + }; + + let executor_profile_id = self.executor_profile_id.clone(); + let mut agent = ExecutorConfigs::get_cached() + .get_coding_agent(&executor_profile_id) + .ok_or(ExecutorError::UnknownExecutorType( + executor_profile_id.to_string(), + ))?; + + agent.use_approvals(approvals.clone()); + + agent + .spawn_review( + &effective_dir, + self.context.as_deref(), + self.additional_prompt.as_deref(), + env, + ) + .await + } +} diff --git a/crates/executors/src/executors/mod.rs b/crates/executors/src/executors/mod.rs index 3b526d957c..532deeef5c 100644 --- a/crates/executors/src/executors/mod.rs +++ b/crates/executors/src/executors/mod.rs @@ -15,7 +15,7 @@ use workspace_utils::msg_store::MsgStore; #[cfg(feature = "qa-mode")] use crate::executors::qa_mock::QaMockExecutor; use crate::{ - actions::ExecutorAction, + actions::{ExecutorAction, review::RepoReviewContext}, approvals::ExecutorApprovalService, command::CommandBuildError, env::ExecutionEnv, @@ -215,6 +215,20 @@ pub trait StandardCodingAgentExecutor { session_id: &str, env: &ExecutionEnv, ) -> Result; + + /// Spawn a review session. Default implementation builds a prompt and delegates to spawn(). + /// Executors with native review support (e.g., Codex) can override this. + async fn spawn_review( + &self, + current_dir: &Path, + context: Option<&[RepoReviewContext]>, + additional_prompt: Option<&str>, + env: &ExecutionEnv, + ) -> Result { + let prompt = build_review_prompt(context, additional_prompt); + self.spawn(current_dir, &prompt, env).await + } + fn normalize_logs(&self, _raw_logs_event_store: Arc, _worktree_path: &Path); // MCP configuration methods @@ -298,6 +312,30 @@ impl AppendPrompt { } } +/// Build a natural language review prompt from context and additional instructions. +pub fn build_review_prompt( + context: Option<&[RepoReviewContext]>, + additional_prompt: Option<&str>, +) -> String { + let mut prompt = String::from("Please review the code changes.\n\n"); + + if let Some(repos) = context { + prompt.push_str("Commits to review:\n"); + for repo in repos { + for hash in &repo.commit_hashes { + prompt.push_str(&format!("- {}\n", hash)); + } + } + prompt.push('\n'); + } + + if let Some(additional) = additional_prompt { + prompt.push_str(additional); + } + + prompt +} + #[cfg(test)] mod tests { use std::str::FromStr; diff --git a/crates/server/src/bin/generate_types.rs b/crates/server/src/bin/generate_types.rs index 50f00dace8..cbb372b659 100644 --- a/crates/server/src/bin/generate_types.rs +++ b/crates/server/src/bin/generate_types.rs @@ -114,6 +114,8 @@ fn generate_types_content() -> String { server::routes::task_attempts::PushTaskAttemptRequest::decl(), server::routes::task_attempts::RenameBranchRequest::decl(), server::routes::task_attempts::RenameBranchResponse::decl(), + server::routes::task_attempts::review::StartReviewRequest::decl(), + server::routes::task_attempts::review::ReviewError::decl(), server::routes::task_attempts::OpenEditorRequest::decl(), server::routes::task_attempts::OpenEditorResponse::decl(), server::routes::shared_tasks::AssignSharedTaskRequest::decl(), @@ -196,6 +198,8 @@ fn generate_types_content() -> String { executors::executors::AppendPrompt::decl(), executors::actions::coding_agent_initial::CodingAgentInitialRequest::decl(), executors::actions::coding_agent_follow_up::CodingAgentFollowUpRequest::decl(), + executors::actions::review::ReviewRequest::decl(), + executors::actions::review::RepoReviewContext::decl(), executors::logs::CommandExitStatus::decl(), executors::logs::CommandRunResult::decl(), executors::logs::NormalizedEntry::decl(), diff --git a/crates/server/src/routes/task_attempts.rs b/crates/server/src/routes/task_attempts.rs index 2c953bcd4f..b48e7497a1 100644 --- a/crates/server/src/routes/task_attempts.rs +++ b/crates/server/src/routes/task_attempts.rs @@ -3,6 +3,7 @@ pub mod cursor_setup; pub mod gh_cli_setup; pub mod images; pub mod pr; +pub mod review; pub mod util; pub mod workspace_summary; @@ -1718,6 +1719,8 @@ pub fn router(deployment: &DeploymentImpl) -> Router { .route("/repos", get(get_task_attempt_repos)) .route("/first-message", get(get_first_user_message)) .route("/mark-seen", put(mark_seen)) + .route("/review", post(review::start_review)) + .route("/", put(update_workspace).delete(delete_workspace)) .layer(from_fn_with_state( deployment.clone(), load_workspace_middleware, diff --git a/crates/server/src/routes/task_attempts/review.rs b/crates/server/src/routes/task_attempts/review.rs new file mode 100644 index 0000000000..1ab668da17 --- /dev/null +++ b/crates/server/src/routes/task_attempts/review.rs @@ -0,0 +1,126 @@ +use axum::{Extension, Json, extract::State, response::Json as ResponseJson}; +use db::models::{ + execution_process::{ExecutionProcess, ExecutionProcessRunReason}, + session::{CreateSession, Session}, + workspace::Workspace, +}; +use deployment::Deployment; +use executors::{ + actions::{ExecutorAction, ExecutorActionType, review::ReviewRequest as ReviewAction}, + profile::ExecutorProfileId, +}; +use serde::{Deserialize, Serialize}; +use services::services::container::ContainerService; +use ts_rs::TS; +use utils::response::ApiResponse; +use uuid::Uuid; + +use crate::{DeploymentImpl, error::ApiError}; + +/// Context for a repository in a review request +#[derive(Debug, Clone, Deserialize, Serialize, TS)] +pub struct RepoReviewContext { + pub repo_id: Uuid, + pub commit_hashes: Vec, +} + +impl From for executors::actions::review::RepoReviewContext { + fn from(ctx: RepoReviewContext) -> Self { + Self { + repo_id: ctx.repo_id, + commit_hashes: ctx.commit_hashes, + } + } +} + +/// Request to start a review session +#[derive(Debug, Deserialize, Serialize, TS)] +pub struct StartReviewRequest { + pub executor_profile_id: ExecutorProfileId, + pub context: Option>, + pub additional_prompt: Option, +} + +/// Error types for review operations +#[derive(Debug, Serialize, Deserialize, TS)] +#[serde(tag = "type", rename_all = "snake_case")] +#[ts(tag = "type", rename_all = "snake_case")] +pub enum ReviewError { + ProcessAlreadyRunning, +} + +#[axum::debug_handler] +pub async fn start_review( + Extension(workspace): Extension, + State(deployment): State, + Json(payload): Json, +) -> Result>, ApiError> { + let pool = &deployment.db().pool; + + // Check if any non-dev-server processes are already running for this workspace + if ExecutionProcess::has_running_non_dev_server_processes_for_workspace(pool, workspace.id) + .await? + { + return Ok(ResponseJson(ApiResponse::error_with_data( + ReviewError::ProcessAlreadyRunning, + ))); + } + + // Ensure container exists + deployment + .container() + .ensure_container_exists(&workspace) + .await?; + + // Create a fresh session for the review + let session = Session::create( + pool, + &CreateSession { + executor: Some(payload.executor_profile_id.executor.to_string()), + }, + Uuid::new_v4(), + workspace.id, + ) + .await?; + + // Convert context types + let context: Option> = payload + .context + .map(|ctx| ctx.into_iter().map(|c| c.into()).collect()); + + // Build the review action + let action = ExecutorAction::new( + ExecutorActionType::ReviewRequest(ReviewAction { + executor_profile_id: payload.executor_profile_id.clone(), + context, + additional_prompt: payload.additional_prompt, + working_dir: workspace.agent_working_dir.clone(), + }), + None, + ); + + // Start execution + let execution_process = deployment + .container() + .start_execution( + &workspace, + &session, + &action, + &ExecutionProcessRunReason::CodingAgent, + ) + .await?; + + // Track analytics + deployment + .track_if_analytics_allowed( + "review_started", + serde_json::json!({ + "workspace_id": workspace.id.to_string(), + "executor": payload.executor_profile_id.executor.to_string(), + "variant": payload.executor_profile_id.variant, + }), + ) + .await; + + Ok(ResponseJson(ApiResponse::success(execution_process))) +} diff --git a/crates/services/src/services/container.rs b/crates/services/src/services/container.rs index 094b2b5ba3..011acf97ce 100644 --- a/crates/services/src/services/container.rs +++ b/crates/services/src/services/container.rs @@ -766,6 +766,11 @@ pub trait ContainerService { ); } } + ExecutorActionType::ReviewRequest(request) => { + let executor = ExecutorConfigs::get_cached() + .get_coding_agent_or_default(&request.executor_profile_id); + executor.normalize_logs(temp_store.clone(), ¤t_dir); + } _ => { tracing::debug!( "Executor action doesn't support log normalization: {:?}", @@ -1117,6 +1122,9 @@ pub trait ContainerService { &request.executor_profile_id, request.effective_dir(&workspace_root), )), + ExecutorActionType::ReviewRequest(request) => { + Some((&request.executor_profile_id, workspace_root.clone())) + } _ => None, } { @@ -1160,13 +1168,15 @@ pub trait ContainerService { } ( ExecutorActionType::CodingAgentInitialRequest(_) - | ExecutorActionType::CodingAgentFollowUpRequest(_), + | ExecutorActionType::CodingAgentFollowUpRequest(_) + | ExecutorActionType::ReviewRequest(_), ExecutorActionType::ScriptRequest(_), ) => ExecutionProcessRunReason::CleanupScript, ( _, ExecutorActionType::CodingAgentFollowUpRequest(_) - | ExecutorActionType::CodingAgentInitialRequest(_), + | ExecutorActionType::CodingAgentInitialRequest(_) + | ExecutorActionType::ReviewRequest(_), ) => ExecutionProcessRunReason::CodingAgent, }; diff --git a/frontend/src/utils/executor.ts b/frontend/src/utils/executor.ts index b83ae1cf55..62c44d729d 100644 --- a/frontend/src/utils/executor.ts +++ b/frontend/src/utils/executor.ts @@ -55,6 +55,7 @@ export function extractProfileFromAction( switch (typ.type) { case 'CodingAgentInitialRequest': case 'CodingAgentFollowUpRequest': + case 'ReviewRequest': return typ.executor_profile_id; case 'ScriptRequest': default: diff --git a/shared/types.ts b/shared/types.ts index c04c9abaf4..d3d2c3c4eb 100644 --- a/shared/types.ts +++ b/shared/types.ts @@ -232,6 +232,10 @@ export type RenameBranchRequest = { new_branch_name: string, }; export type RenameBranchResponse = { branch: string, }; +export type StartReviewRequest = { executor_profile_id: ExecutorProfileId, context: Array | null, additional_prompt: string | null, }; + +export type ReviewError = { "type": "process_already_running" }; + export type OpenEditorRequest = { editor_type: string | null, file_path: string | null, }; export type OpenEditorResponse = { url: string | null, }; @@ -410,7 +414,7 @@ export type ExecutorAction = { typ: ExecutorActionType, next_action: ExecutorAct export type McpConfig = { servers: { [key in string]?: JsonValue }, servers_path: Array, template: JsonValue, preconfigured: JsonValue, is_toml_config: boolean, }; -export type ExecutorActionType = { "type": "CodingAgentInitialRequest" } & CodingAgentInitialRequest | { "type": "CodingAgentFollowUpRequest" } & CodingAgentFollowUpRequest | { "type": "ScriptRequest" } & ScriptRequest; +export type ExecutorActionType = { "type": "CodingAgentInitialRequest" } & CodingAgentInitialRequest | { "type": "CodingAgentFollowUpRequest" } & CodingAgentFollowUpRequest | { "type": "ScriptRequest" } & ScriptRequest | { "type": "ReviewRequest" } & ReviewRequest; export type ScriptContext = "SetupScript" | "CleanupScript" | "DevServer" | "ToolInstallScript"; @@ -515,6 +519,14 @@ executor_profile_id: ExecutorProfileId, */ working_dir: string | null, }; +export type ReviewRequest = { executor_profile_id: ExecutorProfileId, context: Array | null, additional_prompt: string | null, +/** + * Optional relative path to execute the agent in (relative to container_ref). + */ +working_dir: string | null, }; + +export type RepoReviewContext = { repo_id: string, commit_hashes: Array, }; + export type CommandExitStatus = { "type": "exit_code", code: number, } | { "type": "success", success: boolean, }; export type CommandRunResult = { exit_status: CommandExitStatus | null, output: string | null, }; From 725c64bc95fcfc30cf52a75a8b9f74b7af6100b5 Mon Sep 17 00:00:00 2001 From: Alex Netsch Date: Wed, 7 Jan 2026 17:33:55 +0000 Subject: [PATCH 02/54] Backend changes for the `use_all_workspace_commits` flag are complete. Here's what was implemented: Added `use_all_workspace_commits` flag to `StartReviewRequest` for automatically populating review context. 1. **`crates/db/src/models/execution_process_repo_state.rs`** - Added `find_initial_commits_for_workspace()` - query to get the earliest `before_head_commit` for each repo in a workspace 2. **`crates/server/src/routes/task_attempts/review.rs`** - Added `use_all_workspace_commits: bool` field to `StartReviewRequest` (defaults to `false`) - When flag is `true` and `context` is `None`, auto-populates context with initial commits from workspace execution processes 3. **`crates/executors/src/executors/mod.rs`** - Updated `build_review_prompt()` to detect single-commit context (indicating initial commit) - When single commit per repo, prompts agent to "Review all changes made since the following base commit(s)" with git diff hint 4. **`crates/services/src/services/git.rs`** - Added `get_commits_since_branch()` method (may be useful for future use) 5. **`shared/types.ts`** - Generated TypeScript type includes `use_all_workspace_commits: boolean` When frontend calls `POST /task-attempts/{id}/review` with: ```json { "executor_profile_id": { "executor": "CLAUDE_CODE", "variant": null }, "use_all_workspace_commits": true } ``` The backend will: 1. Find the earliest execution process for the workspace 2. Get the `before_head_commit` for each repo (the commit before agents started making changes) 3. Pass that to the prompt builder which tells the agent to review all changes since that commit Cleanup script changes for workspace 38f8b607-9fdc-43dc-afa3-16e8f5e9cbd3 --- ...0a42efb8f38bbb00c504555117743aaa01161.json | 26 +++++++++++ .../models/execution_process_repo_state.rs | 29 ++++++++++++ crates/executors/src/executors/mod.rs | 24 +++++++--- .../server/src/routes/task_attempts/review.rs | 38 ++++++++++++++-- crates/services/src/services/git.rs | 45 +++++++++++++++++++ shared/types.ts | 6 ++- 6 files changed, 158 insertions(+), 10 deletions(-) create mode 100644 crates/db/.sqlx/query-b01828fc0b1d7a6d75821e132ab0a42efb8f38bbb00c504555117743aaa01161.json diff --git a/crates/db/.sqlx/query-b01828fc0b1d7a6d75821e132ab0a42efb8f38bbb00c504555117743aaa01161.json b/crates/db/.sqlx/query-b01828fc0b1d7a6d75821e132ab0a42efb8f38bbb00c504555117743aaa01161.json new file mode 100644 index 0000000000..484e38d100 --- /dev/null +++ b/crates/db/.sqlx/query-b01828fc0b1d7a6d75821e132ab0a42efb8f38bbb00c504555117743aaa01161.json @@ -0,0 +1,26 @@ +{ + "db_name": "SQLite", + "query": "SELECT\n eprs.repo_id as \"repo_id!: Uuid\",\n eprs.before_head_commit as \"before_head_commit!\"\n FROM execution_process_repo_states eprs\n JOIN execution_processes ep ON ep.id = eprs.execution_process_id\n JOIN sessions s ON s.id = ep.session_id\n WHERE s.workspace_id = $1\n AND eprs.before_head_commit IS NOT NULL\n GROUP BY eprs.repo_id\n HAVING ep.created_at = MIN(ep.created_at)", + "describe": { + "columns": [ + { + "name": "repo_id!: Uuid", + "ordinal": 0, + "type_info": "Blob" + }, + { + "name": "before_head_commit!", + "ordinal": 1, + "type_info": "Text" + } + ], + "parameters": { + "Right": 1 + }, + "nullable": [ + false, + true + ] + }, + "hash": "b01828fc0b1d7a6d75821e132ab0a42efb8f38bbb00c504555117743aaa01161" +} diff --git a/crates/db/src/models/execution_process_repo_state.rs b/crates/db/src/models/execution_process_repo_state.rs index 5952979ea2..500cddd8a1 100644 --- a/crates/db/src/models/execution_process_repo_state.rs +++ b/crates/db/src/models/execution_process_repo_state.rs @@ -156,4 +156,33 @@ impl ExecutionProcessRepoState { .fetch_all(pool) .await } + + /// Find the initial before_head_commit for each repo in a workspace. + /// Returns the before_head_commit from the earliest execution process for each repo. + pub async fn find_initial_commits_for_workspace( + pool: &SqlitePool, + workspace_id: Uuid, + ) -> Result, sqlx::Error> { + // For each repo, find the before_head_commit from the earliest process + let rows = sqlx::query!( + r#"SELECT + eprs.repo_id as "repo_id!: Uuid", + eprs.before_head_commit as "before_head_commit!" + FROM execution_process_repo_states eprs + JOIN execution_processes ep ON ep.id = eprs.execution_process_id + JOIN sessions s ON s.id = ep.session_id + WHERE s.workspace_id = $1 + AND eprs.before_head_commit IS NOT NULL + GROUP BY eprs.repo_id + HAVING ep.created_at = MIN(ep.created_at)"#, + workspace_id + ) + .fetch_all(pool) + .await?; + + Ok(rows + .into_iter() + .map(|r| (r.repo_id, r.before_head_commit)) + .collect()) + } } diff --git a/crates/executors/src/executors/mod.rs b/crates/executors/src/executors/mod.rs index 532deeef5c..7e206d1e3d 100644 --- a/crates/executors/src/executors/mod.rs +++ b/crates/executors/src/executors/mod.rs @@ -313,6 +313,7 @@ impl AppendPrompt { } /// Build a natural language review prompt from context and additional instructions. +/// When context contains initial commits, instructs the agent to review all changes from those commits. pub fn build_review_prompt( context: Option<&[RepoReviewContext]>, additional_prompt: Option<&str>, @@ -320,13 +321,26 @@ pub fn build_review_prompt( let mut prompt = String::from("Please review the code changes.\n\n"); if let Some(repos) = context { - prompt.push_str("Commits to review:\n"); - for repo in repos { - for hash in &repo.commit_hashes { - prompt.push_str(&format!("- {}\n", hash)); + // Check if this looks like initial commit context (single commit per repo) + let has_single_commits = repos.iter().all(|r| r.commit_hashes.len() == 1); + + if has_single_commits && !repos.is_empty() { + prompt.push_str("Review all changes made since the following base commit(s):\n"); + for repo in repos { + if let Some(hash) = repo.commit_hashes.first() { + prompt.push_str(&format!("- {} (base)\n", hash)); + } } + prompt.push_str("\nYou can use `git diff ..HEAD` to see the changes.\n\n"); + } else { + prompt.push_str("Commits to review:\n"); + for repo in repos { + for hash in &repo.commit_hashes { + prompt.push_str(&format!("- {}\n", hash)); + } + } + prompt.push('\n'); } - prompt.push('\n'); } if let Some(additional) = additional_prompt { diff --git a/crates/server/src/routes/task_attempts/review.rs b/crates/server/src/routes/task_attempts/review.rs index 1ab668da17..226bce1ab2 100644 --- a/crates/server/src/routes/task_attempts/review.rs +++ b/crates/server/src/routes/task_attempts/review.rs @@ -1,6 +1,7 @@ use axum::{Extension, Json, extract::State, response::Json as ResponseJson}; use db::models::{ execution_process::{ExecutionProcess, ExecutionProcessRunReason}, + execution_process_repo_state::ExecutionProcessRepoState, session::{CreateSession, Session}, workspace::Workspace, }; @@ -39,6 +40,9 @@ pub struct StartReviewRequest { pub executor_profile_id: ExecutorProfileId, pub context: Option>, pub additional_prompt: Option, + /// If true and context is None, automatically include all workspace commits + #[serde(default)] + pub use_all_workspace_commits: bool, } /// Error types for review operations @@ -83,10 +87,36 @@ pub async fn start_review( ) .await?; - // Convert context types - let context: Option> = payload - .context - .map(|ctx| ctx.into_iter().map(|c| c.into()).collect()); + // Build context - either from payload or auto-populated from workspace commits + let context: Option> = + if let Some(ctx) = payload.context { + // Use explicit context if provided + Some(ctx.into_iter().map(|c| c.into()).collect()) + } else if payload.use_all_workspace_commits { + // Auto-populate with initial commits for each repo in the workspace + let initial_commits = + ExecutionProcessRepoState::find_initial_commits_for_workspace(pool, workspace.id) + .await?; + + if initial_commits.is_empty() { + None + } else { + Some( + initial_commits + .into_iter() + .map(|(repo_id, initial_commit)| { + executors::actions::review::RepoReviewContext { + repo_id, + // Store just the initial commit - prompt will say "from this commit onwards" + commit_hashes: vec![initial_commit], + } + }) + .collect(), + ) + } + } else { + None + }; // Build the review action let action = ExecutorAction::new( diff --git a/crates/services/src/services/git.rs b/crates/services/src/services/git.rs index 5562ec0a8b..9dc6449e62 100644 --- a/crates/services/src/services/git.rs +++ b/crates/services/src/services/git.rs @@ -1869,4 +1869,49 @@ impl GitService { Ok(stats) } + + /// Get commits on current_branch that are not on target_branch. + /// Returns list of commit SHAs (oldest first). + pub fn get_commits_since_branch( + &self, + repo_path: &Path, + current_branch: &str, + target_branch: &str, + ) -> Result, GitServiceError> { + let repo = self.open_repo(repo_path)?; + + // Find the current branch HEAD + let current_ref = repo + .find_branch(current_branch, BranchType::Local)? + .get() + .target() + .ok_or_else(|| GitServiceError::BranchNotFound(current_branch.to_string()))?; + + // Find the target branch HEAD - try local first, then remote + let target_oid = if let Ok(branch) = repo.find_branch(target_branch, BranchType::Local) { + branch + .get() + .target() + .ok_or_else(|| GitServiceError::BranchNotFound(target_branch.to_string()))? + } else { + // Try as remote branch (e.g., "origin/main") + let remote_ref = format!("refs/remotes/{}", target_branch); + repo.refname_to_id(&remote_ref) + .map_err(|_| GitServiceError::BranchNotFound(target_branch.to_string()))? + }; + + // Set up revwalk + let mut revwalk = repo.revwalk()?; + revwalk.push(current_ref)?; + revwalk.hide(target_oid)?; + revwalk.set_sorting(Sort::REVERSE | Sort::TIME)?; // Oldest first + + // Collect commit SHAs + let commits: Vec = revwalk + .filter_map(|oid_result| oid_result.ok()) + .map(|oid| oid.to_string()) + .collect(); + + Ok(commits) + } } diff --git a/shared/types.ts b/shared/types.ts index d3d2c3c4eb..8756126854 100644 --- a/shared/types.ts +++ b/shared/types.ts @@ -232,7 +232,11 @@ export type RenameBranchRequest = { new_branch_name: string, }; export type RenameBranchResponse = { branch: string, }; -export type StartReviewRequest = { executor_profile_id: ExecutorProfileId, context: Array | null, additional_prompt: string | null, }; +export type StartReviewRequest = { executor_profile_id: ExecutorProfileId, context: Array | null, additional_prompt: string | null, +/** + * If true and context is None, automatically include all workspace commits + */ +use_all_workspace_commits: boolean, }; export type ReviewError = { "type": "process_already_running" }; From 672c1eba2c8b3323ceda29bf1c66c047d60b0d97 Mon Sep 17 00:00:00 2001 From: Alex Netsch Date: Wed, 7 Jan 2026 17:42:36 +0000 Subject: [PATCH 03/54] Frontend UI implementation is complete. Here's a summary of what was added: Added simple frontend UI to test the review endpoint. 1. **`frontend/src/lib/api.ts`** - Added `startReview` method to `attemptsApi` - Imports `StartReviewRequest`, `ReviewError` types 2. **`frontend/src/hooks/useStartReview.ts`** (NEW) - Hook that calls `attemptsApi.startReview` with `use_all_workspace_commits: true` - Invalidates `processes` and `branchStatus` queries on success 3. **`frontend/src/hooks/index.ts`** - Exports `useStartReview` 4. **`frontend/src/components/ui/actions-dropdown.tsx`** - Added "Start Review" menu item in the attempt actions section - Uses the user's default executor profile from config - Shows loading state while starting review 5. **`frontend/src/i18n/locales/en/tasks.json`** - Added translation keys: `startReview`, `startingReview` From the workspace actions dropdown (three dots menu), click "Start Review" to start a code review session using all workspace commits as context. Done! The fix has been implemented. The review endpoint now uses `Session::find_latest_by_workspace_id` to find an existing session for the workspace, and only creates a new session if none exists. This means review logs will now appear in the same conversation view as other coding agent runs. **Change summary:** - Modified `crates/server/src/routes/task_attempts/review.rs` (lines 79-93) - Instead of always creating a new session, it now checks for an existing session first using `Session::find_latest_by_workspace_id` - This matches the pattern used by dev server, setup scripts, and other similar endpoints Done! The fix is now in place. The change adds `ReviewRequest` to the action type filter in `useConversationHistory.ts` so review processes are included in the conversation view, and handles the content extraction appropriately (using `additional_prompt` for reviews instead of `prompt`). **Change summary:** - Modified `frontend/src/hooks/useConversationHistory.ts` (lines 226-244) - Added `ReviewRequest` to the action type filter - Used conditional to extract content from `additional_prompt` for reviews (with fallback "Review the code changes") vs `prompt` for coding agent requests Cleanup script changes for workspace 38f8b607-9fdc-43dc-afa3-16e8f5e9cbd3 Done! All fixes are now in place: 1. **`useConversationHistory.ts` - `flattenEntries()`** (line 177-184): Added `ReviewRequest` to the filter so review processes are included in the flattened entries 2. **`useConversationHistory.ts` - `flattenEntriesForEmit()`** (line 226-244): Already fixed earlier - handles `ReviewRequest` and extracts `additional_prompt` for user message content 3. **`ProcessesTab.tsx`** (line 223-228): Added `ReviewRequest` to the ProfileVariantBadge conditional so the agent badge shows for review processes --- .../server/src/routes/task_attempts/review.rs | 25 +++++++----- .../tasks/TaskDetails/ProcessesTab.tsx | 4 +- .../src/components/ui/actions-dropdown.tsx | 28 +++++++++++++ frontend/src/hooks/index.ts | 1 + frontend/src/hooks/useConversationHistory.ts | 12 ++++-- frontend/src/hooks/useStartReview.ts | 39 +++++++++++++++++++ frontend/src/i18n/locales/en/tasks.json | 2 + frontend/src/lib/api.ts | 18 +++++++++ 8 files changed, 115 insertions(+), 14 deletions(-) create mode 100644 frontend/src/hooks/useStartReview.ts diff --git a/crates/server/src/routes/task_attempts/review.rs b/crates/server/src/routes/task_attempts/review.rs index 226bce1ab2..bdeac439ce 100644 --- a/crates/server/src/routes/task_attempts/review.rs +++ b/crates/server/src/routes/task_attempts/review.rs @@ -76,16 +76,21 @@ pub async fn start_review( .ensure_container_exists(&workspace) .await?; - // Create a fresh session for the review - let session = Session::create( - pool, - &CreateSession { - executor: Some(payload.executor_profile_id.executor.to_string()), - }, - Uuid::new_v4(), - workspace.id, - ) - .await?; + // Use existing session if available (so review logs appear in same conversation view) + let session = match Session::find_latest_by_workspace_id(pool, workspace.id).await? { + Some(s) => s, + None => { + Session::create( + pool, + &CreateSession { + executor: Some(payload.executor_profile_id.executor.to_string()), + }, + Uuid::new_v4(), + workspace.id, + ) + .await? + } + }; // Build context - either from payload or auto-populated from workspace commits let context: Option> = diff --git a/frontend/src/components/tasks/TaskDetails/ProcessesTab.tsx b/frontend/src/components/tasks/TaskDetails/ProcessesTab.tsx index 226f1ecc61..74c4104b4e 100644 --- a/frontend/src/components/tasks/TaskDetails/ProcessesTab.tsx +++ b/frontend/src/components/tasks/TaskDetails/ProcessesTab.tsx @@ -223,7 +223,9 @@ function ProcessesTab({ sessionId }: ProcessesTabProps) { {process.executor_action.typ.type === 'CodingAgentInitialRequest' || process.executor_action.typ.type === - 'CodingAgentFollowUpRequest' ? ( + 'CodingAgentFollowUpRequest' || + process.executor_action.typ.type === + 'ReviewRequest' ? ( { + e.stopPropagation(); + if (!attempt?.id || !config?.executor_profile) return; + try { + await startReview.mutateAsync({ + executorProfileId: config.executor_profile, + }); + } catch { + // Error handled in hook + } + }; + return ( <> @@ -222,6 +238,18 @@ export function ActionsDropdown({ > {t('actionsMenu.editBranchName')} + + {startReview.isPending + ? t('actionsMenu.startingReview') + : t('actionsMenu.startReview')} + )} diff --git a/frontend/src/hooks/index.ts b/frontend/src/hooks/index.ts index 0ea3c73686..e43a937849 100644 --- a/frontend/src/hooks/index.ts +++ b/frontend/src/hooks/index.ts @@ -33,3 +33,4 @@ export { useOrganizationInvitations } from './useOrganizationInvitations'; export { useOrganizationMutations } from './useOrganizationMutations'; export { useVariant } from './useVariant'; export { useRetryProcess } from './useRetryProcess'; +export { useStartReview } from './useStartReview'; diff --git a/frontend/src/hooks/useConversationHistory.ts b/frontend/src/hooks/useConversationHistory.ts index d6acafa094..49ca336cb8 100644 --- a/frontend/src/hooks/useConversationHistory.ts +++ b/frontend/src/hooks/useConversationHistory.ts @@ -179,7 +179,8 @@ export const useConversationHistory = ({ p.executionProcess.executor_action.typ.type === 'CodingAgentFollowUpRequest' || p.executionProcess.executor_action.typ.type === - 'CodingAgentInitialRequest' + 'CodingAgentInitialRequest' || + p.executionProcess.executor_action.typ.type === 'ReviewRequest' ) .sort( (a, b) => @@ -227,14 +228,19 @@ export const useConversationHistory = ({ p.executionProcess.executor_action.typ.type === 'CodingAgentInitialRequest' || p.executionProcess.executor_action.typ.type === - 'CodingAgentFollowUpRequest' + 'CodingAgentFollowUpRequest' || + p.executionProcess.executor_action.typ.type === 'ReviewRequest' ) { // New user message + const actionType = p.executionProcess.executor_action.typ; const userNormalizedEntry: NormalizedEntry = { entry_type: { type: 'user_message', }, - content: p.executionProcess.executor_action.typ.prompt, + content: + actionType.type === 'ReviewRequest' + ? (actionType.additional_prompt ?? 'Review the code changes') + : actionType.prompt, timestamp: null, }; const userPatch: PatchType = { diff --git a/frontend/src/hooks/useStartReview.ts b/frontend/src/hooks/useStartReview.ts new file mode 100644 index 0000000000..c4bdf28abb --- /dev/null +++ b/frontend/src/hooks/useStartReview.ts @@ -0,0 +1,39 @@ +import { useMutation, useQueryClient } from '@tanstack/react-query'; +import { attemptsApi } from '@/lib/api'; +import type { ExecutorProfileId } from 'shared/types'; + +type StartReviewParams = { + executorProfileId: ExecutorProfileId; + additionalPrompt?: string; +}; + +export function useStartReview( + attemptId?: string, + onSuccess?: () => void, + onError?: (err: unknown) => void +) { + const queryClient = useQueryClient(); + + return useMutation({ + mutationFn: async (params: StartReviewParams) => { + if (!attemptId) throw new Error('No attempt ID'); + return attemptsApi.startReview(attemptId, { + executor_profile_id: params.executorProfileId, + context: null, + additional_prompt: params.additionalPrompt ?? null, + use_all_workspace_commits: true, + }); + }, + onSuccess: () => { + // Refresh processes to show the new review session + queryClient.invalidateQueries({ queryKey: ['processes', attemptId] }); + // Refresh branch status + queryClient.invalidateQueries({ queryKey: ['branchStatus', attemptId] }); + onSuccess?.(); + }, + onError: (err) => { + console.error('Failed to start review:', err); + onError?.(err); + }, + }); +} diff --git a/frontend/src/i18n/locales/en/tasks.json b/frontend/src/i18n/locales/en/tasks.json index 82901c34f4..901c379e9c 100644 --- a/frontend/src/i18n/locales/en/tasks.json +++ b/frontend/src/i18n/locales/en/tasks.json @@ -465,6 +465,8 @@ "createSubtask": "Create subtask", "gitActions": "Git actions", "editBranchName": "Edit branch name", + "startReview": "Start Review", + "startingReview": "Starting Review...", "task": "Task", "share": "Share", "reassign": "Reassign", diff --git a/frontend/src/lib/api.ts b/frontend/src/lib/api.ts index 8419dcca24..1bf7091d7f 100644 --- a/frontend/src/lib/api.ts +++ b/frontend/src/lib/api.ts @@ -88,6 +88,8 @@ import { AbortConflictsRequest, Session, Workspace, + StartReviewRequest, + ReviewError, } from 'shared/types'; import type { WorkspaceWithSession } from '@/types/attempt'; import { createWorkspaceWithSession } from '@/types/attempt'; @@ -538,6 +540,22 @@ export const attemptsApi = { return handleApiResponse(response); }, + startReview: async ( + attemptId: string, + data: StartReviewRequest + ): Promise> => { + const response = await makeRequest( + `/api/task-attempts/${attemptId}/review`, + { + method: 'POST', + body: JSON.stringify(data), + } + ); + return handleApiResponse>( + response + ); + }, + delete: async (attemptId: string): Promise => { const response = await makeRequest(`/api/task-attempts/${attemptId}`, { method: 'DELETE', From dd47b5135e93162b02bec68fbe34deb60ff67c06 Mon Sep 17 00:00:00 2001 From: Alex Netsch Date: Wed, 7 Jan 2026 18:39:08 +0000 Subject: [PATCH 04/54] Done. Changes made: 1. **`crates/executors/src/actions/review.rs`**: Added `prompt: String` field to `ReviewRequest` 2. **`crates/server/src/routes/task_attempts/review.rs`**: - Import `build_review_prompt` - Build the full prompt from context and additional_prompt before creating the action 3. **`frontend/src/hooks/useConversationHistory.ts`**: Simplified content extraction to just use `actionType.prompt` for all action types (since `ReviewRequest` now has a prompt field too) 4. **Regenerated TypeScript types** - `ReviewRequest` in `shared/types.ts` now includes the `prompt` field Cleanup script changes for workspace 38f8b607-9fdc-43dc-afa3-16e8f5e9cbd3 --- crates/executors/src/actions/review.rs | 2 ++ crates/server/src/routes/task_attempts/review.rs | 5 +++++ frontend/src/hooks/useConversationHistory.ts | 5 +---- shared/types.ts | 4 ++++ 4 files changed, 12 insertions(+), 4 deletions(-) diff --git a/crates/executors/src/actions/review.rs b/crates/executors/src/actions/review.rs index 8b6431f21d..36db95247c 100644 --- a/crates/executors/src/actions/review.rs +++ b/crates/executors/src/actions/review.rs @@ -26,6 +26,8 @@ pub struct ReviewRequest { pub executor_profile_id: ExecutorProfileId, pub context: Option>, pub additional_prompt: Option, + /// The full prompt sent to the model (built from context + additional_prompt) + pub prompt: String, /// Optional relative path to execute the agent in (relative to container_ref). #[serde(default)] pub working_dir: Option, diff --git a/crates/server/src/routes/task_attempts/review.rs b/crates/server/src/routes/task_attempts/review.rs index bdeac439ce..9884f62a6f 100644 --- a/crates/server/src/routes/task_attempts/review.rs +++ b/crates/server/src/routes/task_attempts/review.rs @@ -8,6 +8,7 @@ use db::models::{ use deployment::Deployment; use executors::{ actions::{ExecutorAction, ExecutorActionType, review::ReviewRequest as ReviewAction}, + executors::build_review_prompt, profile::ExecutorProfileId, }; use serde::{Deserialize, Serialize}; @@ -123,12 +124,16 @@ pub async fn start_review( None }; + // Build the full prompt for display and execution + let prompt = build_review_prompt(context.as_deref(), payload.additional_prompt.as_deref()); + // Build the review action let action = ExecutorAction::new( ExecutorActionType::ReviewRequest(ReviewAction { executor_profile_id: payload.executor_profile_id.clone(), context, additional_prompt: payload.additional_prompt, + prompt, working_dir: workspace.agent_working_dir.clone(), }), None, diff --git a/frontend/src/hooks/useConversationHistory.ts b/frontend/src/hooks/useConversationHistory.ts index 49ca336cb8..3cb3f8e2bb 100644 --- a/frontend/src/hooks/useConversationHistory.ts +++ b/frontend/src/hooks/useConversationHistory.ts @@ -237,10 +237,7 @@ export const useConversationHistory = ({ entry_type: { type: 'user_message', }, - content: - actionType.type === 'ReviewRequest' - ? (actionType.additional_prompt ?? 'Review the code changes') - : actionType.prompt, + content: actionType.prompt, timestamp: null, }; const userPatch: PatchType = { diff --git a/shared/types.ts b/shared/types.ts index 8756126854..1e1e357e61 100644 --- a/shared/types.ts +++ b/shared/types.ts @@ -524,6 +524,10 @@ executor_profile_id: ExecutorProfileId, working_dir: string | null, }; export type ReviewRequest = { executor_profile_id: ExecutorProfileId, context: Array | null, additional_prompt: string | null, +/** + * The full prompt sent to the model (built from context + additional_prompt) + */ +prompt: string, /** * Optional relative path to execute the agent in (relative to container_ref). */ From 46927d66fe7214529e1d367a7cc63356ec1fc72d Mon Sep 17 00:00:00 2001 From: Alex Netsch Date: Wed, 7 Jan 2026 18:48:05 +0000 Subject: [PATCH 05/54] Done. Here's a summary of the changes: **Backend:** 1. **`crates/executors/src/actions/review.rs`**: - Added `CommitRange` enum with three variants: `FromBase`, `Specific`, and `Range` - Updated `RepoReviewContext` to use `repo_name: String` and `commits: CommitRange` instead of `commit_hashes` 2. **`crates/executors/src/executors/mod.rs`**: - Updated `build_review_prompt` to format output with repo names and handle all `CommitRange` variants 3. **`crates/server/src/routes/task_attempts/review.rs`**: - Simplified `StartReviewRequest` (removed `context` field, kept `use_all_workspace_commits`) - Updated handler to look up repo names via `Repo::find_by_ids` and use `CommitRange::FromBase` 4. **`crates/server/src/bin/generate_types.rs`**: Added `CommitRange` to exported types **Frontend:** - **`frontend/src/hooks/useStartReview.ts`**: Removed `context: null` since that field no longer exists The prompt will now look like: ``` Please review the code changes. Repository: vibe-kanban Review all changes from base commit abc123 to HEAD. Use `git diff abc123..HEAD` to see the changes. ``` Cleanup script changes for workspace 38f8b607-9fdc-43dc-afa3-16e8f5e9cbd3 --- crates/executors/src/actions/review.rs | 16 +++- crates/executors/src/executors/mod.rs | 46 ++++++---- crates/server/src/bin/generate_types.rs | 1 + .../server/src/routes/task_attempts/review.rs | 89 +++++++++---------- frontend/src/hooks/useStartReview.ts | 1 - shared/types.ts | 8 +- 6 files changed, 93 insertions(+), 68 deletions(-) diff --git a/crates/executors/src/actions/review.rs b/crates/executors/src/actions/review.rs index 36db95247c..9158a6b3a0 100644 --- a/crates/executors/src/actions/review.rs +++ b/crates/executors/src/actions/review.rs @@ -13,11 +13,25 @@ use crate::{ profile::{ExecutorConfigs, ExecutorProfileId}, }; +/// Specifies which commits to review +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, TS)] +#[serde(tag = "type", rename_all = "snake_case")] +#[ts(tag = "type", rename_all = "snake_case")] +pub enum CommitRange { + /// All commits from this base commit to HEAD + FromBase { commit: String }, + /// Specific commits to review + Specific { commits: Vec }, + /// Range from one commit to another + Range { from: String, to: String }, +} + /// Context for a repository in a review request #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, TS)] pub struct RepoReviewContext { pub repo_id: Uuid, - pub commit_hashes: Vec, + pub repo_name: String, + pub commits: CommitRange, } /// Request to start a code review session diff --git a/crates/executors/src/executors/mod.rs b/crates/executors/src/executors/mod.rs index 7e206d1e3d..faa3306899 100644 --- a/crates/executors/src/executors/mod.rs +++ b/crates/executors/src/executors/mod.rs @@ -313,30 +313,44 @@ impl AppendPrompt { } /// Build a natural language review prompt from context and additional instructions. -/// When context contains initial commits, instructs the agent to review all changes from those commits. pub fn build_review_prompt( context: Option<&[RepoReviewContext]>, additional_prompt: Option<&str>, ) -> String { + use crate::actions::review::CommitRange; + let mut prompt = String::from("Please review the code changes.\n\n"); if let Some(repos) = context { - // Check if this looks like initial commit context (single commit per repo) - let has_single_commits = repos.iter().all(|r| r.commit_hashes.len() == 1); - - if has_single_commits && !repos.is_empty() { - prompt.push_str("Review all changes made since the following base commit(s):\n"); - for repo in repos { - if let Some(hash) = repo.commit_hashes.first() { - prompt.push_str(&format!("- {} (base)\n", hash)); + for repo in repos { + prompt.push_str(&format!("Repository: {}\n", repo.repo_name)); + + match &repo.commits { + CommitRange::FromBase { commit } => { + prompt.push_str(&format!( + "Review all changes from base commit {} to HEAD.\n", + commit + )); + prompt.push_str(&format!( + "Use `git diff {}..HEAD` to see the changes.\n", + commit + )); } - } - prompt.push_str("\nYou can use `git diff ..HEAD` to see the changes.\n\n"); - } else { - prompt.push_str("Commits to review:\n"); - for repo in repos { - for hash in &repo.commit_hashes { - prompt.push_str(&format!("- {}\n", hash)); + CommitRange::Specific { commits } => { + prompt.push_str("Review the following commits:\n"); + for hash in commits { + prompt.push_str(&format!("- {}\n", hash)); + } + } + CommitRange::Range { from, to } => { + prompt.push_str(&format!( + "Review all changes from commit {} to {}.\n", + from, to + )); + prompt.push_str(&format!( + "Use `git diff {}..{}` to see the changes.\n", + from, to + )); } } prompt.push('\n'); diff --git a/crates/server/src/bin/generate_types.rs b/crates/server/src/bin/generate_types.rs index cbb372b659..c64f7941e1 100644 --- a/crates/server/src/bin/generate_types.rs +++ b/crates/server/src/bin/generate_types.rs @@ -200,6 +200,7 @@ fn generate_types_content() -> String { executors::actions::coding_agent_follow_up::CodingAgentFollowUpRequest::decl(), executors::actions::review::ReviewRequest::decl(), executors::actions::review::RepoReviewContext::decl(), + executors::actions::review::CommitRange::decl(), executors::logs::CommandExitStatus::decl(), executors::logs::CommandRunResult::decl(), executors::logs::NormalizedEntry::decl(), diff --git a/crates/server/src/routes/task_attempts/review.rs b/crates/server/src/routes/task_attempts/review.rs index 9884f62a6f..4f00d8d1e6 100644 --- a/crates/server/src/routes/task_attempts/review.rs +++ b/crates/server/src/routes/task_attempts/review.rs @@ -2,12 +2,19 @@ use axum::{Extension, Json, extract::State, response::Json as ResponseJson}; use db::models::{ execution_process::{ExecutionProcess, ExecutionProcessRunReason}, execution_process_repo_state::ExecutionProcessRepoState, + repo::Repo, session::{CreateSession, Session}, workspace::Workspace, }; use deployment::Deployment; use executors::{ - actions::{ExecutorAction, ExecutorActionType, review::ReviewRequest as ReviewAction}, + actions::{ + ExecutorAction, ExecutorActionType, + review::{ + CommitRange, RepoReviewContext as ExecutorRepoReviewContext, + ReviewRequest as ReviewAction, + }, + }, executors::build_review_prompt, profile::ExecutorProfileId, }; @@ -19,29 +26,12 @@ use uuid::Uuid; use crate::{DeploymentImpl, error::ApiError}; -/// Context for a repository in a review request -#[derive(Debug, Clone, Deserialize, Serialize, TS)] -pub struct RepoReviewContext { - pub repo_id: Uuid, - pub commit_hashes: Vec, -} - -impl From for executors::actions::review::RepoReviewContext { - fn from(ctx: RepoReviewContext) -> Self { - Self { - repo_id: ctx.repo_id, - commit_hashes: ctx.commit_hashes, - } - } -} - /// Request to start a review session #[derive(Debug, Deserialize, Serialize, TS)] pub struct StartReviewRequest { pub executor_profile_id: ExecutorProfileId, - pub context: Option>, pub additional_prompt: Option, - /// If true and context is None, automatically include all workspace commits + /// If true, automatically include all workspace commits from initial state #[serde(default)] pub use_all_workspace_commits: bool, } @@ -93,36 +83,41 @@ pub async fn start_review( } }; - // Build context - either from payload or auto-populated from workspace commits - let context: Option> = - if let Some(ctx) = payload.context { - // Use explicit context if provided - Some(ctx.into_iter().map(|c| c.into()).collect()) - } else if payload.use_all_workspace_commits { - // Auto-populate with initial commits for each repo in the workspace - let initial_commits = - ExecutionProcessRepoState::find_initial_commits_for_workspace(pool, workspace.id) - .await?; + // Build context - auto-populated from workspace commits when requested + let context: Option> = if payload.use_all_workspace_commits { + // Auto-populate with initial commits for each repo in the workspace + let initial_commits = + ExecutionProcessRepoState::find_initial_commits_for_workspace(pool, workspace.id) + .await?; - if initial_commits.is_empty() { - None - } else { - Some( - initial_commits - .into_iter() - .map(|(repo_id, initial_commit)| { - executors::actions::review::RepoReviewContext { - repo_id, - // Store just the initial commit - prompt will say "from this commit onwards" - commit_hashes: vec![initial_commit], - } - }) - .collect(), - ) - } - } else { + if initial_commits.is_empty() { None - }; + } else { + // Look up repo names + let repo_ids: Vec = initial_commits.iter().map(|(id, _)| *id).collect(); + let repos = Repo::find_by_ids(pool, &repo_ids).await?; + let repo_map: std::collections::HashMap = + repos.iter().map(|r| (r.id, r)).collect(); + + Some( + initial_commits + .into_iter() + .filter_map(|(repo_id, initial_commit)| { + let repo = repo_map.get(&repo_id)?; + Some(ExecutorRepoReviewContext { + repo_id, + repo_name: repo.display_name.clone(), + commits: CommitRange::FromBase { + commit: initial_commit, + }, + }) + }) + .collect(), + ) + } + } else { + None + }; // Build the full prompt for display and execution let prompt = build_review_prompt(context.as_deref(), payload.additional_prompt.as_deref()); diff --git a/frontend/src/hooks/useStartReview.ts b/frontend/src/hooks/useStartReview.ts index c4bdf28abb..39fabea836 100644 --- a/frontend/src/hooks/useStartReview.ts +++ b/frontend/src/hooks/useStartReview.ts @@ -19,7 +19,6 @@ export function useStartReview( if (!attemptId) throw new Error('No attempt ID'); return attemptsApi.startReview(attemptId, { executor_profile_id: params.executorProfileId, - context: null, additional_prompt: params.additionalPrompt ?? null, use_all_workspace_commits: true, }); diff --git a/shared/types.ts b/shared/types.ts index 1e1e357e61..a0f6d9f6a4 100644 --- a/shared/types.ts +++ b/shared/types.ts @@ -232,9 +232,9 @@ export type RenameBranchRequest = { new_branch_name: string, }; export type RenameBranchResponse = { branch: string, }; -export type StartReviewRequest = { executor_profile_id: ExecutorProfileId, context: Array | null, additional_prompt: string | null, +export type StartReviewRequest = { executor_profile_id: ExecutorProfileId, additional_prompt: string | null, /** - * If true and context is None, automatically include all workspace commits + * If true, automatically include all workspace commits from initial state */ use_all_workspace_commits: boolean, }; @@ -533,7 +533,9 @@ prompt: string, */ working_dir: string | null, }; -export type RepoReviewContext = { repo_id: string, commit_hashes: Array, }; +export type RepoReviewContext = { repo_id: string, repo_name: string, commits: CommitRange, }; + +export type CommitRange = { "type": "from_base", commit: string, } | { "type": "specific", commits: Array, } | { "type": "range", from: string, to: string, }; export type CommandExitStatus = { "type": "exit_code", code: number, } | { "type": "success", success: boolean, }; From 01b246f010fd9c1c6e5c877dfac384dafb05171b Mon Sep 17 00:00:00 2001 From: Alex Netsch Date: Thu, 8 Jan 2026 10:09:26 +0000 Subject: [PATCH 06/54] Codex review support (vibe-kanban e7996a18) ## Context We've added a review endpoint (\`POST /task-attempts/{id}/review\`) that starts a code review session. Currently it uses a default prompt-based implementation via \`spawn\_review()\` on \`StandardCodingAgentExecutor\`, which builds a prompt and delegates to \`spawn()\`. ## Goal Implement native Codex review support by overriding \`spawn\_review()\` in the Codex executor to use Codex's native review mechanism instead of the prompt-based approach. ## Current Implementation ### Backend Types \*\*\`CommitRange\` enum\*\* (\`crates/executors/src/actions/review.rs\`): \`\`\`rust pub enum CommitRange { FromBase { commit: String }, Specific { commits: Vec }, Range { from: String, to: String }, } \`\`\` \*\*\`RepoReviewContext\`\*\*: \`\`\`rust pub struct RepoReviewContext { pub repo\_id: Uuid, pub repo\_name: String, pub commits: CommitRange, } \`\`\` ### Trait Method \*\*\`spawn\_review()\`\*\* in \`StandardCodingAgentExecutor\` trait (\`crates/executors/src/executors/mod.rs\`): \`\`\`rust async fn spawn\_review( &self, current\_dir: &Path, context: Option<&[RepoReviewContext]>, additional\_prompt: Option<&str>, env: &ExecutionEnv, ) -> Result { // Default: build prompt and delegate to spawn() let prompt = build\_review\_prompt(context, additional\_prompt); self.spawn(current\_dir, &prompt, env).await } \`\`\` ## Tasks 1. Research Codex's native review API/mechanism (if it has one) 2. Override \`spawn\_review()\` in \`CodexExecutor\` to use native review if available 3. Fall back to prompt-based approach if native review isn't supported --- crates/executors/src/executors/codex.rs | 84 ++++++-- .../executors/src/executors/codex/client.rs | 23 ++- .../executors/src/executors/codex/review.rs | 189 ++++++++++++++++++ 3 files changed, 276 insertions(+), 20 deletions(-) create mode 100644 crates/executors/src/executors/codex/review.rs diff --git a/crates/executors/src/executors/codex.rs b/crates/executors/src/executors/codex.rs index 00f77018e4..efc648c2d1 100644 --- a/crates/executors/src/executors/codex.rs +++ b/crates/executors/src/executors/codex.rs @@ -1,6 +1,7 @@ pub mod client; pub mod jsonrpc; pub mod normalize_logs; +pub mod review; pub mod session; use std::{ collections::HashMap, @@ -9,7 +10,7 @@ use std::{ }; use async_trait::async_trait; -use codex_app_server_protocol::NewConversationParams; +use codex_app_server_protocol::{NewConversationParams, ReviewTarget}; use codex_protocol::{ config_types::SandboxMode as CodexSandboxMode, protocol::AskForApproval as CodexAskForApproval, }; @@ -30,6 +31,7 @@ use self::{ session::SessionHandler, }; use crate::{ + actions::review::RepoReviewContext, approvals::ExecutorApprovalService, command::{CmdOverrides, CommandBuilder, CommandParts, apply_overrides}, env::ExecutionEnv, @@ -102,6 +104,11 @@ pub enum ReasoningSummaryFormat { Experimental, } +enum CodexSessionAction { + Chat { prompt: String }, + Review { target: ReviewTarget }, +} + #[derive(Derivative, Clone, Serialize, Deserialize, TS, JsonSchema)] #[derivative(Debug, PartialEq)] pub struct Codex { @@ -155,7 +162,11 @@ impl StandardCodingAgentExecutor for Codex { env: &ExecutionEnv, ) -> Result { let command_parts = self.build_command_builder().build_initial()?; - self.spawn_inner(current_dir, prompt, command_parts, None, env) + let combined_prompt = self.append_prompt.combine_prompt(prompt); + let action = CodexSessionAction::Chat { + prompt: combined_prompt, + }; + self.spawn_inner(current_dir, command_parts, action, None, env) .await } @@ -167,7 +178,11 @@ impl StandardCodingAgentExecutor for Codex { env: &ExecutionEnv, ) -> Result { let command_parts = self.build_command_builder().build_follow_up(&[])?; - self.spawn_inner(current_dir, prompt, command_parts, Some(session_id), env) + let combined_prompt = self.append_prompt.combine_prompt(prompt); + let action = CodexSessionAction::Chat { + prompt: combined_prompt, + }; + self.spawn_inner(current_dir, command_parts, action, Some(session_id), env) .await } @@ -206,6 +221,22 @@ impl StandardCodingAgentExecutor for Codex { AvailabilityInfo::NotFound } } + + async fn spawn_review( + &self, + current_dir: &Path, + context: Option<&[RepoReviewContext]>, + additional_prompt: Option<&str>, + env: &ExecutionEnv, + ) -> Result { + let command_parts = self.build_command_builder().build_initial()?; + let review_target = review::map_to_review_target(context, additional_prompt); + let action = CodexSessionAction::Review { + target: review_target, + }; + self.spawn_inner(current_dir, command_parts, action, None, env) + .await + } } impl Codex { @@ -294,12 +325,11 @@ impl Codex { async fn spawn_inner( &self, current_dir: &Path, - prompt: &str, command_parts: CommandParts, + action: CodexSessionAction, resume_session: Option<&str>, env: &ExecutionEnv, ) -> Result { - let combined_prompt = self.append_prompt.combine_prompt(prompt); let (program_path, args) = command_parts.into_resolved().await?; let mut process = Command::new(program_path); @@ -340,19 +370,37 @@ impl Codex { tokio::spawn(async move { let exit_signal_tx = ExitSignalSender::new(exit_signal_tx); let log_writer = LogWriter::new(new_stdout); - if let Err(err) = Self::launch_codex_app_server( - params, - resume_session, - combined_prompt, - child_stdout, - child_stdin, - log_writer.clone(), - exit_signal_tx.clone(), - approvals, - auto_approve, - ) - .await - { + let launch_result = match action { + CodexSessionAction::Chat { prompt } => { + Self::launch_codex_app_server( + params, + resume_session, + prompt, + child_stdout, + child_stdin, + log_writer.clone(), + exit_signal_tx.clone(), + approvals, + auto_approve, + ) + .await + } + CodexSessionAction::Review { target } => { + review::launch_codex_review( + params, + resume_session, + target, + child_stdout, + child_stdin, + log_writer.clone(), + exit_signal_tx.clone(), + approvals, + auto_approve, + ) + .await + } + }; + if let Err(err) = launch_result { match &err { ExecutorError::Io(io_err) if io_err.kind() == std::io::ErrorKind::BrokenPipe => diff --git a/crates/executors/src/executors/codex/client.rs b/crates/executors/src/executors/codex/client.rs index 535cc0ce81..bb90ca2bbb 100644 --- a/crates/executors/src/executors/codex/client.rs +++ b/crates/executors/src/executors/codex/client.rs @@ -12,7 +12,8 @@ use codex_app_server_protocol::{ GetAuthStatusParams, GetAuthStatusResponse, InitializeParams, InitializeResponse, InputItem, JSONRPCError, JSONRPCNotification, JSONRPCRequest, JSONRPCResponse, NewConversationParams, NewConversationResponse, RequestId, ResumeConversationParams, ResumeConversationResponse, - SendUserMessageParams, SendUserMessageResponse, ServerNotification, ServerRequest, + ReviewStartParams, ReviewStartResponse, ReviewTarget, SendUserMessageParams, + SendUserMessageResponse, ServerNotification, ServerRequest, }; use codex_protocol::{ConversationId, protocol::ReviewDecision}; use serde::{Serialize, de::DeserializeOwned}; @@ -146,6 +147,23 @@ impl AppServerClient { }; self.send_request(request, "getAuthStatus").await } + + pub async fn start_review( + &self, + thread_id: String, + target: ReviewTarget, + ) -> Result { + let request = ClientRequest::ReviewStart { + request_id: self.next_request_id(), + params: ReviewStartParams { + thread_id, + target, + delivery: None, + }, + }; + self.send_request(request, "reviewStart").await + } + async fn handle_server_request( &self, peer: &JsonRpcPeer, @@ -482,7 +500,8 @@ fn request_id(request: &ClientRequest) -> RequestId { | ClientRequest::GetAuthStatus { request_id, .. } | ClientRequest::ResumeConversation { request_id, .. } | ClientRequest::AddConversationListener { request_id, .. } - | ClientRequest::SendUserMessage { request_id, .. } => request_id.clone(), + | ClientRequest::SendUserMessage { request_id, .. } + | ClientRequest::ReviewStart { request_id, .. } => request_id.clone(), _ => unreachable!("request_id called for unsupported request variant"), } } diff --git a/crates/executors/src/executors/codex/review.rs b/crates/executors/src/executors/codex/review.rs new file mode 100644 index 0000000000..1160ce9b08 --- /dev/null +++ b/crates/executors/src/executors/codex/review.rs @@ -0,0 +1,189 @@ +//! Review-specific functionality for the Codex executor. + +use std::sync::Arc; + +use codex_app_server_protocol::{NewConversationParams, ReviewTarget}; + +use super::{ + client::{AppServerClient, LogWriter}, + jsonrpc::{ExitSignalSender, JsonRpcPeer}, + session::SessionHandler, +}; +use crate::{ + actions::review::{CommitRange, RepoReviewContext}, + approvals::ExecutorApprovalService, + executors::ExecutorError, +}; + +/// Launch a Codex review session. +#[allow(clippy::too_many_arguments)] +pub async fn launch_codex_review( + conversation_params: NewConversationParams, + resume_session: Option, + review_target: ReviewTarget, + child_stdout: tokio::process::ChildStdout, + child_stdin: tokio::process::ChildStdin, + log_writer: LogWriter, + exit_signal_tx: ExitSignalSender, + approvals: Option>, + auto_approve: bool, +) -> Result<(), ExecutorError> { + let client = AppServerClient::new(log_writer, approvals, auto_approve); + let rpc_peer = JsonRpcPeer::spawn(child_stdin, child_stdout, client.clone(), exit_signal_tx); + client.connect(rpc_peer); + client.initialize().await?; + let auth_status = client.get_auth_status().await?; + if auth_status.requires_openai_auth.unwrap_or(true) && auth_status.auth_method.is_none() { + return Err(ExecutorError::AuthRequired( + "Codex authentication required".to_string(), + )); + } + + let conversation_id = match resume_session { + Some(session_id) => { + let (rollout_path, _forked_session_id) = SessionHandler::fork_rollout_file(&session_id) + .map_err(|e| ExecutorError::FollowUpNotSupported(e.to_string()))?; + let response = client + .resume_conversation(rollout_path.clone(), conversation_params) + .await?; + tracing::debug!( + "resuming session for review using rollout file {}, response {:?}", + rollout_path.display(), + response + ); + response.conversation_id + } + None => { + let response = client.new_conversation(conversation_params).await?; + response.conversation_id + } + }; + + client.register_session(&conversation_id).await?; + client.add_conversation_listener(conversation_id).await?; + + client + .start_review(conversation_id.to_string(), review_target) + .await?; + + Ok(()) +} + +/// Map review context and additional prompt to a Codex ReviewTarget. +pub fn map_to_review_target( + context: Option<&[RepoReviewContext]>, + additional_prompt: Option<&str>, +) -> ReviewTarget { + // If no context provided, use Custom with additional_prompt or UncommittedChanges + let Some(repos) = context else { + return match additional_prompt { + Some(prompt) if !prompt.trim().is_empty() => ReviewTarget::Custom { + instructions: prompt.to_string(), + }, + _ => ReviewTarget::UncommittedChanges, + }; + }; + + if repos.is_empty() { + return match additional_prompt { + Some(prompt) if !prompt.trim().is_empty() => ReviewTarget::Custom { + instructions: prompt.to_string(), + }, + _ => ReviewTarget::UncommittedChanges, + }; + } + + // For multiple repos or complex scenarios, build Custom instructions + if repos.len() > 1 { + let mut instructions = String::new(); + for repo in repos { + instructions.push_str(&format!("Repository: {}\n", repo.repo_name)); + match &repo.commits { + CommitRange::FromBase { commit } => { + instructions.push_str(&format!( + "Review all changes from base commit {} to HEAD.\n", + commit + )); + } + CommitRange::Specific { commits } => { + instructions.push_str("Review the following commits:\n"); + for hash in commits { + instructions.push_str(&format!("- {}\n", hash)); + } + } + CommitRange::Range { from, to } => { + instructions.push_str(&format!( + "Review all changes from commit {} to {}.\n", + from, to + )); + } + } + instructions.push('\n'); + } + if let Some(prompt) = additional_prompt { + instructions.push_str(prompt); + } + return ReviewTarget::Custom { instructions }; + } + + // Single repo - map to native ReviewTarget where possible + let repo = &repos[0]; + match &repo.commits { + CommitRange::FromBase { commit } => { + // Map to BaseBranch - the commit serves as the base reference + match additional_prompt { + Some(prompt) if !prompt.trim().is_empty() => { + // With additional prompt, use Custom + ReviewTarget::Custom { + instructions: format!( + "Review all changes from base commit {} to HEAD in {}.\n{}", + commit, repo.repo_name, prompt + ), + } + } + _ => ReviewTarget::BaseBranch { + branch: commit.clone(), + }, + } + } + CommitRange::Specific { commits } => { + if commits.len() == 1 { + // Single commit - map to Commit + match additional_prompt { + Some(prompt) if !prompt.trim().is_empty() => ReviewTarget::Custom { + instructions: format!( + "Review commit {} in {}.\n{}", + commits[0], repo.repo_name, prompt + ), + }, + _ => ReviewTarget::Commit { + sha: commits[0].clone(), + title: None, + }, + } + } else { + // Multiple commits - use Custom + let mut instructions = format!("Repository: {}\n", repo.repo_name); + instructions.push_str("Review the following commits:\n"); + for hash in commits { + instructions.push_str(&format!("- {}\n", hash)); + } + if let Some(prompt) = additional_prompt { + instructions.push_str(prompt); + } + ReviewTarget::Custom { instructions } + } + } + CommitRange::Range { from, to } => { + // Range doesn't have a direct mapping, use Custom + let mut instructions = format!( + "Repository: {}\nReview all changes from commit {} to {}.\n", + repo.repo_name, from, to + ); + if let Some(prompt) = additional_prompt { + instructions.push_str(prompt); + } + ReviewTarget::Custom { instructions } + } + } +} From 5f8c51f0cb47759099963a257d71d67fd8584c9e Mon Sep 17 00:00:00 2001 From: Alex Netsch Date: Thu, 8 Jan 2026 11:18:12 +0000 Subject: [PATCH 07/54] Done. Here's a summary of the changes: 1. **Removed dead code** from `crates/executors/src/executors/codex/review.rs`: - Deleted the `map_to_review_target()` function (was ~120 lines) - Removed unused imports (`CommitRange`, `RepoReviewContext`) 2. **Codex now uses `ReviewTarget::Custom`** with the pre-built prompt, which ensures the displayed prompt matches what Codex actually receives. The build passes and TypeScript types have been regenerated. --- crates/executors/src/actions/review.rs | 10 ++++++---- crates/executors/src/executors/mod.rs | 12 +++++++----- crates/server/src/routes/task_attempts/review.rs | 2 +- shared/types.ts | 8 ++++++-- 4 files changed, 20 insertions(+), 12 deletions(-) diff --git a/crates/executors/src/actions/review.rs b/crates/executors/src/actions/review.rs index 9158a6b3a0..9122eea429 100644 --- a/crates/executors/src/actions/review.rs +++ b/crates/executors/src/actions/review.rs @@ -39,9 +39,11 @@ pub struct RepoReviewContext { pub struct ReviewRequest { pub executor_profile_id: ExecutorProfileId, pub context: Option>, - pub additional_prompt: Option, - /// The full prompt sent to the model (built from context + additional_prompt) + /// The full prompt sent to the model pub prompt: String, + /// Optional session ID to resume an existing session + #[serde(default)] + pub session_id: Option, /// Optional relative path to execute the agent in (relative to container_ref). #[serde(default)] pub working_dir: Option, @@ -79,8 +81,8 @@ impl Executable for ReviewRequest { agent .spawn_review( &effective_dir, - self.context.as_deref(), - self.additional_prompt.as_deref(), + &self.prompt, + self.session_id.as_deref(), env, ) .await diff --git a/crates/executors/src/executors/mod.rs b/crates/executors/src/executors/mod.rs index faa3306899..f73495022b 100644 --- a/crates/executors/src/executors/mod.rs +++ b/crates/executors/src/executors/mod.rs @@ -216,17 +216,19 @@ pub trait StandardCodingAgentExecutor { env: &ExecutionEnv, ) -> Result; - /// Spawn a review session. Default implementation builds a prompt and delegates to spawn(). + /// Spawn a review session. Default implementation delegates to spawn() or spawn_follow_up(). /// Executors with native review support (e.g., Codex) can override this. async fn spawn_review( &self, current_dir: &Path, - context: Option<&[RepoReviewContext]>, - additional_prompt: Option<&str>, + prompt: &str, + session_id: Option<&str>, env: &ExecutionEnv, ) -> Result { - let prompt = build_review_prompt(context, additional_prompt); - self.spawn(current_dir, &prompt, env).await + match session_id { + Some(id) => self.spawn_follow_up(current_dir, prompt, id, env).await, + None => self.spawn(current_dir, prompt, env).await, + } } fn normalize_logs(&self, _raw_logs_event_store: Arc, _worktree_path: &Path); diff --git a/crates/server/src/routes/task_attempts/review.rs b/crates/server/src/routes/task_attempts/review.rs index 4f00d8d1e6..66ad04b1b4 100644 --- a/crates/server/src/routes/task_attempts/review.rs +++ b/crates/server/src/routes/task_attempts/review.rs @@ -127,8 +127,8 @@ pub async fn start_review( ExecutorActionType::ReviewRequest(ReviewAction { executor_profile_id: payload.executor_profile_id.clone(), context, - additional_prompt: payload.additional_prompt, prompt, + session_id: None, // TODO: wire up from StartReviewRequest if needed working_dir: workspace.agent_working_dir.clone(), }), None, diff --git a/shared/types.ts b/shared/types.ts index a0f6d9f6a4..fd309ed305 100644 --- a/shared/types.ts +++ b/shared/types.ts @@ -523,11 +523,15 @@ executor_profile_id: ExecutorProfileId, */ working_dir: string | null, }; -export type ReviewRequest = { executor_profile_id: ExecutorProfileId, context: Array | null, additional_prompt: string | null, +export type ReviewRequest = { executor_profile_id: ExecutorProfileId, context: Array | null, /** - * The full prompt sent to the model (built from context + additional_prompt) + * The full prompt sent to the model */ prompt: string, +/** + * Optional session ID to resume an existing session + */ +session_id: string | null, /** * Optional relative path to execute the agent in (relative to container_ref). */ From 4b3345e9884180bd606b89c309247c84f9276cdd Mon Sep 17 00:00:00 2001 From: Alex Netsch Date: Thu, 8 Jan 2026 11:18:29 +0000 Subject: [PATCH 08/54] Use custom review target for codex --- crates/executors/src/executors/codex.rs | 12 +- .../executors/src/executors/codex/review.rs | 125 +----------------- 2 files changed, 8 insertions(+), 129 deletions(-) diff --git a/crates/executors/src/executors/codex.rs b/crates/executors/src/executors/codex.rs index efc648c2d1..325b7cb624 100644 --- a/crates/executors/src/executors/codex.rs +++ b/crates/executors/src/executors/codex.rs @@ -31,7 +31,6 @@ use self::{ session::SessionHandler, }; use crate::{ - actions::review::RepoReviewContext, approvals::ExecutorApprovalService, command::{CmdOverrides, CommandBuilder, CommandParts, apply_overrides}, env::ExecutionEnv, @@ -225,16 +224,19 @@ impl StandardCodingAgentExecutor for Codex { async fn spawn_review( &self, current_dir: &Path, - context: Option<&[RepoReviewContext]>, - additional_prompt: Option<&str>, + prompt: &str, + session_id: Option<&str>, env: &ExecutionEnv, ) -> Result { let command_parts = self.build_command_builder().build_initial()?; - let review_target = review::map_to_review_target(context, additional_prompt); + // Use Custom review target with the pre-built prompt + let review_target = ReviewTarget::Custom { + instructions: prompt.to_string(), + }; let action = CodexSessionAction::Review { target: review_target, }; - self.spawn_inner(current_dir, command_parts, action, None, env) + self.spawn_inner(current_dir, command_parts, action, session_id, env) .await } } diff --git a/crates/executors/src/executors/codex/review.rs b/crates/executors/src/executors/codex/review.rs index 1160ce9b08..a9ef1ad4a6 100644 --- a/crates/executors/src/executors/codex/review.rs +++ b/crates/executors/src/executors/codex/review.rs @@ -9,11 +9,7 @@ use super::{ jsonrpc::{ExitSignalSender, JsonRpcPeer}, session::SessionHandler, }; -use crate::{ - actions::review::{CommitRange, RepoReviewContext}, - approvals::ExecutorApprovalService, - executors::ExecutorError, -}; +use crate::{approvals::ExecutorApprovalService, executors::ExecutorError}; /// Launch a Codex review session. #[allow(clippy::too_many_arguments)] @@ -68,122 +64,3 @@ pub async fn launch_codex_review( Ok(()) } - -/// Map review context and additional prompt to a Codex ReviewTarget. -pub fn map_to_review_target( - context: Option<&[RepoReviewContext]>, - additional_prompt: Option<&str>, -) -> ReviewTarget { - // If no context provided, use Custom with additional_prompt or UncommittedChanges - let Some(repos) = context else { - return match additional_prompt { - Some(prompt) if !prompt.trim().is_empty() => ReviewTarget::Custom { - instructions: prompt.to_string(), - }, - _ => ReviewTarget::UncommittedChanges, - }; - }; - - if repos.is_empty() { - return match additional_prompt { - Some(prompt) if !prompt.trim().is_empty() => ReviewTarget::Custom { - instructions: prompt.to_string(), - }, - _ => ReviewTarget::UncommittedChanges, - }; - } - - // For multiple repos or complex scenarios, build Custom instructions - if repos.len() > 1 { - let mut instructions = String::new(); - for repo in repos { - instructions.push_str(&format!("Repository: {}\n", repo.repo_name)); - match &repo.commits { - CommitRange::FromBase { commit } => { - instructions.push_str(&format!( - "Review all changes from base commit {} to HEAD.\n", - commit - )); - } - CommitRange::Specific { commits } => { - instructions.push_str("Review the following commits:\n"); - for hash in commits { - instructions.push_str(&format!("- {}\n", hash)); - } - } - CommitRange::Range { from, to } => { - instructions.push_str(&format!( - "Review all changes from commit {} to {}.\n", - from, to - )); - } - } - instructions.push('\n'); - } - if let Some(prompt) = additional_prompt { - instructions.push_str(prompt); - } - return ReviewTarget::Custom { instructions }; - } - - // Single repo - map to native ReviewTarget where possible - let repo = &repos[0]; - match &repo.commits { - CommitRange::FromBase { commit } => { - // Map to BaseBranch - the commit serves as the base reference - match additional_prompt { - Some(prompt) if !prompt.trim().is_empty() => { - // With additional prompt, use Custom - ReviewTarget::Custom { - instructions: format!( - "Review all changes from base commit {} to HEAD in {}.\n{}", - commit, repo.repo_name, prompt - ), - } - } - _ => ReviewTarget::BaseBranch { - branch: commit.clone(), - }, - } - } - CommitRange::Specific { commits } => { - if commits.len() == 1 { - // Single commit - map to Commit - match additional_prompt { - Some(prompt) if !prompt.trim().is_empty() => ReviewTarget::Custom { - instructions: format!( - "Review commit {} in {}.\n{}", - commits[0], repo.repo_name, prompt - ), - }, - _ => ReviewTarget::Commit { - sha: commits[0].clone(), - title: None, - }, - } - } else { - // Multiple commits - use Custom - let mut instructions = format!("Repository: {}\n", repo.repo_name); - instructions.push_str("Review the following commits:\n"); - for hash in commits { - instructions.push_str(&format!("- {}\n", hash)); - } - if let Some(prompt) = additional_prompt { - instructions.push_str(prompt); - } - ReviewTarget::Custom { instructions } - } - } - CommitRange::Range { from, to } => { - // Range doesn't have a direct mapping, use Custom - let mut instructions = format!( - "Repository: {}\nReview all changes from commit {} to {}.\n", - repo.repo_name, from, to - ); - if let Some(prompt) = additional_prompt { - instructions.push_str(prompt); - } - ReviewTarget::Custom { instructions } - } - } -} From 2a500c0e0e88931eed8398f0472e923b475c0e44 Mon Sep 17 00:00:00 2001 From: Alex Netsch Date: Thu, 8 Jan 2026 11:43:20 +0000 Subject: [PATCH 09/54] Done. Here's a summary of the changes: MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Moved the review endpoint from workspace-level to session-level: ### Changes 1. **Created `crates/server/src/routes/sessions/review.rs`** - New review endpoint at `POST /sessions/{id}/review` - Takes `Extension` instead of `Extension` - Looks up `agent_session_id` from previous executions using `find_latest_coding_agent_turn_session_id()` - Passes `agent_session_id` to `ReviewAction` for session resumption 2. **Updated `crates/server/src/routes/sessions/mod.rs`** - Added `pub mod review;` - Added route `.route("/review", post(review::start_review))` 3. **Updated `crates/server/src/routes/task_attempts.rs`** - Removed `pub mod review;` - Removed `.route("/review", post(review::start_review))` 4. **Deleted `crates/server/src/routes/task_attempts/review.rs`** 5. **Updated `crates/server/src/bin/generate_types.rs`** - Changed path from `task_attempts::review` to `sessions::review` ### API Change - **Old**: `POST /api/task-attempts/{workspace_id}/review` - **New**: `POST /api/sessions/{session_id}/review` This ensures vk sessions and agent sessions are 1:1: - Existing session → resumes agent session if available - New session → fresh agent session --- crates/server/src/bin/generate_types.rs | 4 +- crates/server/src/routes/sessions/mod.rs | 2 + .../{task_attempts => sessions}/review.rs | 38 +++++++++---------- crates/server/src/routes/task_attempts.rs | 2 - 4 files changed, 23 insertions(+), 23 deletions(-) rename crates/server/src/routes/{task_attempts => sessions}/review.rs (84%) diff --git a/crates/server/src/bin/generate_types.rs b/crates/server/src/bin/generate_types.rs index c64f7941e1..a50fc238f5 100644 --- a/crates/server/src/bin/generate_types.rs +++ b/crates/server/src/bin/generate_types.rs @@ -114,8 +114,8 @@ fn generate_types_content() -> String { server::routes::task_attempts::PushTaskAttemptRequest::decl(), server::routes::task_attempts::RenameBranchRequest::decl(), server::routes::task_attempts::RenameBranchResponse::decl(), - server::routes::task_attempts::review::StartReviewRequest::decl(), - server::routes::task_attempts::review::ReviewError::decl(), + server::routes::sessions::review::StartReviewRequest::decl(), + server::routes::sessions::review::ReviewError::decl(), server::routes::task_attempts::OpenEditorRequest::decl(), server::routes::task_attempts::OpenEditorResponse::decl(), server::routes::shared_tasks::AssignSharedTaskRequest::decl(), diff --git a/crates/server/src/routes/sessions/mod.rs b/crates/server/src/routes/sessions/mod.rs index ccf689b4db..7ab4de381c 100644 --- a/crates/server/src/routes/sessions/mod.rs +++ b/crates/server/src/routes/sessions/mod.rs @@ -1,4 +1,5 @@ pub mod queue; +pub mod review; use std::str::FromStr; @@ -239,6 +240,7 @@ pub fn router(deployment: &DeploymentImpl) -> Router { let session_id_router = Router::new() .route("/", get(get_session)) .route("/follow-up", post(follow_up)) + .route("/review", post(review::start_review)) .layer(from_fn_with_state( deployment.clone(), load_session_middleware, diff --git a/crates/server/src/routes/task_attempts/review.rs b/crates/server/src/routes/sessions/review.rs similarity index 84% rename from crates/server/src/routes/task_attempts/review.rs rename to crates/server/src/routes/sessions/review.rs index 66ad04b1b4..6b8ca82dc3 100644 --- a/crates/server/src/routes/task_attempts/review.rs +++ b/crates/server/src/routes/sessions/review.rs @@ -3,8 +3,8 @@ use db::models::{ execution_process::{ExecutionProcess, ExecutionProcessRunReason}, execution_process_repo_state::ExecutionProcessRepoState, repo::Repo, - session::{CreateSession, Session}, - workspace::Workspace, + session::Session, + workspace::{Workspace, WorkspaceError}, }; use deployment::Deployment; use executors::{ @@ -46,12 +46,19 @@ pub enum ReviewError { #[axum::debug_handler] pub async fn start_review( - Extension(workspace): Extension, + Extension(session): Extension, State(deployment): State, Json(payload): Json, ) -> Result>, ApiError> { let pool = &deployment.db().pool; + // Load workspace from session + let workspace = Workspace::find_by_id(pool, session.workspace_id) + .await? + .ok_or(ApiError::Workspace(WorkspaceError::ValidationError( + "Workspace not found".to_string(), + )))?; + // Check if any non-dev-server processes are already running for this workspace if ExecutionProcess::has_running_non_dev_server_processes_for_workspace(pool, workspace.id) .await? @@ -67,21 +74,9 @@ pub async fn start_review( .ensure_container_exists(&workspace) .await?; - // Use existing session if available (so review logs appear in same conversation view) - let session = match Session::find_latest_by_workspace_id(pool, workspace.id).await? { - Some(s) => s, - None => { - Session::create( - pool, - &CreateSession { - executor: Some(payload.executor_profile_id.executor.to_string()), - }, - Uuid::new_v4(), - workspace.id, - ) - .await? - } - }; + // Lookup agent session_id from previous execution in this session (for session resumption) + let agent_session_id = + ExecutionProcess::find_latest_coding_agent_turn_session_id(pool, session.id).await?; // Build context - auto-populated from workspace commits when requested let context: Option> = if payload.use_all_workspace_commits { @@ -122,13 +117,16 @@ pub async fn start_review( // Build the full prompt for display and execution let prompt = build_review_prompt(context.as_deref(), payload.additional_prompt.as_deref()); + // Track whether we're resuming a session (before moving agent_session_id) + let resumed_session = agent_session_id.is_some(); + // Build the review action let action = ExecutorAction::new( ExecutorActionType::ReviewRequest(ReviewAction { executor_profile_id: payload.executor_profile_id.clone(), context, prompt, - session_id: None, // TODO: wire up from StartReviewRequest if needed + session_id: agent_session_id, working_dir: workspace.agent_working_dir.clone(), }), None, @@ -151,8 +149,10 @@ pub async fn start_review( "review_started", serde_json::json!({ "workspace_id": workspace.id.to_string(), + "session_id": session.id.to_string(), "executor": payload.executor_profile_id.executor.to_string(), "variant": payload.executor_profile_id.variant, + "resumed_session": resumed_session, }), ) .await; diff --git a/crates/server/src/routes/task_attempts.rs b/crates/server/src/routes/task_attempts.rs index b48e7497a1..83a8d12489 100644 --- a/crates/server/src/routes/task_attempts.rs +++ b/crates/server/src/routes/task_attempts.rs @@ -3,7 +3,6 @@ pub mod cursor_setup; pub mod gh_cli_setup; pub mod images; pub mod pr; -pub mod review; pub mod util; pub mod workspace_summary; @@ -1719,7 +1718,6 @@ pub fn router(deployment: &DeploymentImpl) -> Router { .route("/repos", get(get_task_attempt_repos)) .route("/first-message", get(get_first_user_message)) .route("/mark-seen", put(mark_seen)) - .route("/review", post(review::start_review)) .route("/", put(update_workspace).delete(delete_workspace)) .layer(from_fn_with_state( deployment.clone(), From 5838c5fa0a7d6b4f80612535363ad83c7299b232 Mon Sep 17 00:00:00 2001 From: Alex Netsch Date: Thu, 8 Jan 2026 12:04:09 +0000 Subject: [PATCH 10/54] Done! Here's a summary of all the frontend changes: - Added `startReview` to `sessionsApi` using new endpoint `POST /api/sessions/${sessionId}/review` - Removed `startReview` from `attemptsApi` - Changed parameter from `attemptId` to `sessionId` and added optional `workspaceId` for query invalidation - Changed API call from `attemptsApi.startReview` to `sessionsApi.startReview` - Added `MagnifyingGlassIcon` import - Added `onStartReview` and `isReviewStarting` to `SessionProps` interface - Added "Start Review" menu item in the session dropdown - Added `useStartReview` hook import and usage - Added `handleStartReview` callback function - Passed `onStartReview` and `isReviewStarting` to `SessionChatBox` - Removed `useStartReview` import - Removed `useUserSystem` import (now unused) - Removed `startReview` hook usage - Removed `handleStartReview` function - Removed "Start Review" menu item from the dropdown --- .../containers/SessionChatBoxContainer.tsx | 22 ++++++++++++++ .../ui-new/primitives/SessionChatBox.tsx | 17 +++++++++++ .../src/components/ui/actions-dropdown.tsx | 28 ------------------ frontend/src/hooks/useStartReview.ts | 19 +++++++----- frontend/src/lib/api.ts | 29 +++++++++---------- 5 files changed, 64 insertions(+), 51 deletions(-) diff --git a/frontend/src/components/ui-new/containers/SessionChatBoxContainer.tsx b/frontend/src/components/ui-new/containers/SessionChatBoxContainer.tsx index 1d70599423..bedf60bfb1 100644 --- a/frontend/src/components/ui-new/containers/SessionChatBoxContainer.tsx +++ b/frontend/src/components/ui-new/containers/SessionChatBoxContainer.tsx @@ -22,6 +22,7 @@ import { useSessionAttachments } from '@/hooks/useSessionAttachments'; import { useMessageEditRetry } from '@/hooks/useMessageEditRetry'; import { useBranchStatus } from '@/hooks/useBranchStatus'; import { useApprovalMutation } from '@/hooks/useApprovalMutation'; +import { useStartReview } from '@/hooks/useStartReview'; import { workspaceSummaryKeys } from '@/components/ui-new/hooks/useWorkspaces'; import { SessionChatBox, @@ -155,6 +156,9 @@ export function SessionChatBoxContainer({ const { approveAsync, denyAsync, isApproving, isDenying, denyError } = useApprovalMutation(); + // Review mutation for starting reviews + const startReviewMutation = useStartReview(sessionId, workspaceId); + // Branch status for edit retry and conflict detection const { data: branchStatus } = useBranchStatus(workspaceId); @@ -493,6 +497,22 @@ export function SessionChatBoxContainer({ ? new Date() > new Date(pendingApproval.timeoutAt) : false; + // Handle start review + const handleStartReview = useCallback(async () => { + if (!latestProfileId) return; + + try { + await startReviewMutation.mutateAsync({ + executorProfileId: { + executor: latestProfileId.executor, + variant: latestProfileId.variant, + }, + }); + } catch { + // Error is handled by mutation + } + }, [latestProfileId, startReviewMutation]); + // Compute execution status const status = computeExecutionStatus({ isInFeedbackMode, @@ -549,6 +569,8 @@ export function SessionChatBoxContainer({ onSelectSession: onSelectSession ?? (() => {}), isNewSessionMode, onNewSession: onStartNewSession, + onStartReview: handleStartReview, + isReviewStarting: startReviewMutation.isPending, }} stats={{ filesChanged, diff --git a/frontend/src/components/ui-new/primitives/SessionChatBox.tsx b/frontend/src/components/ui-new/primitives/SessionChatBox.tsx index 1dd52c6387..4716a68cfd 100644 --- a/frontend/src/components/ui-new/primitives/SessionChatBox.tsx +++ b/frontend/src/components/ui-new/primitives/SessionChatBox.tsx @@ -9,6 +9,7 @@ import { ChatCircleIcon, TrashIcon, WarningIcon, + MagnifyingGlassIcon, } from '@phosphor-icons/react'; import { useTranslation } from 'react-i18next'; import type { Session, BaseCodingAgent, TodoItem } from 'shared/types'; @@ -61,6 +62,10 @@ interface SessionProps { isNewSessionMode?: boolean; /** Callback to start new session mode */ onNewSession?: () => void; + /** Callback to start a review */ + onStartReview?: () => void; + /** Whether review is currently starting */ + isReviewStarting?: boolean; } interface StatsProps { @@ -242,6 +247,8 @@ export function SessionChatBox({ onSelectSession, isNewSessionMode, onNewSession, + onStartReview, + isReviewStarting, } = session; const isLatestSelected = sessions.length > 0 && selectedSessionId === sessions[0].id; @@ -587,6 +594,16 @@ export function SessionChatBox({ > {t('conversation.sessions.newSession')} + {/* Start Review option */} + {onStartReview && !isNewSessionMode && ( + onStartReview()} + disabled={isRunning || isReviewStarting} + > + {isReviewStarting ? 'Starting Review...' : 'Start Review'} + + )} {sessions.length > 0 && } {sessions.length > 0 ? ( <> diff --git a/frontend/src/components/ui/actions-dropdown.tsx b/frontend/src/components/ui/actions-dropdown.tsx index c2e813c593..410c5bd209 100644 --- a/frontend/src/components/ui/actions-dropdown.tsx +++ b/frontend/src/components/ui/actions-dropdown.tsx @@ -11,8 +11,6 @@ import { import { MoreHorizontal } from 'lucide-react'; import type { TaskWithAttemptStatus } from 'shared/types'; import { useOpenInEditor } from '@/hooks/useOpenInEditor'; -import { useStartReview } from '@/hooks/useStartReview'; -import { useUserSystem } from '@/components/ConfigProvider'; import { DeleteTaskConfirmationDialog } from '@/components/dialogs/tasks/DeleteTaskConfirmationDialog'; import { ViewProcessesDialog } from '@/components/dialogs/tasks/ViewProcessesDialog'; import { ViewRelatedTasksDialog } from '@/components/dialogs/tasks/ViewRelatedTasksDialog'; @@ -46,8 +44,6 @@ export function ActionsDropdown({ const openInEditor = useOpenInEditor(attempt?.id); const navigate = useNavigate(); const { userId, isSignedIn } = useAuth(); - const { config } = useUserSystem(); - const startReview = useStartReview(attempt?.id); const hasAttemptActions = Boolean(attempt); const hasTaskActions = Boolean(task); @@ -169,18 +165,6 @@ export function ActionsDropdown({ const canStopShare = Boolean(sharedTask) && sharedTask?.assignee_user_id === userId; - const handleStartReview = async (e: React.MouseEvent) => { - e.stopPropagation(); - if (!attempt?.id || !config?.executor_profile) return; - try { - await startReview.mutateAsync({ - executorProfileId: config.executor_profile, - }); - } catch { - // Error handled in hook - } - }; - return ( <> @@ -238,18 +222,6 @@ export function ActionsDropdown({ > {t('actionsMenu.editBranchName')} - - {startReview.isPending - ? t('actionsMenu.startingReview') - : t('actionsMenu.startReview')} - )} diff --git a/frontend/src/hooks/useStartReview.ts b/frontend/src/hooks/useStartReview.ts index 39fabea836..52c845ffba 100644 --- a/frontend/src/hooks/useStartReview.ts +++ b/frontend/src/hooks/useStartReview.ts @@ -1,5 +1,5 @@ import { useMutation, useQueryClient } from '@tanstack/react-query'; -import { attemptsApi } from '@/lib/api'; +import { sessionsApi } from '@/lib/api'; import type { ExecutorProfileId } from 'shared/types'; type StartReviewParams = { @@ -8,7 +8,8 @@ type StartReviewParams = { }; export function useStartReview( - attemptId?: string, + sessionId?: string, + workspaceId?: string, onSuccess?: () => void, onError?: (err: unknown) => void ) { @@ -16,8 +17,8 @@ export function useStartReview( return useMutation({ mutationFn: async (params: StartReviewParams) => { - if (!attemptId) throw new Error('No attempt ID'); - return attemptsApi.startReview(attemptId, { + if (!sessionId) throw new Error('No session ID'); + return sessionsApi.startReview(sessionId, { executor_profile_id: params.executorProfileId, additional_prompt: params.additionalPrompt ?? null, use_all_workspace_commits: true, @@ -25,9 +26,13 @@ export function useStartReview( }, onSuccess: () => { // Refresh processes to show the new review session - queryClient.invalidateQueries({ queryKey: ['processes', attemptId] }); - // Refresh branch status - queryClient.invalidateQueries({ queryKey: ['branchStatus', attemptId] }); + if (workspaceId) { + queryClient.invalidateQueries({ queryKey: ['processes', workspaceId] }); + // Refresh branch status + queryClient.invalidateQueries({ + queryKey: ['branchStatus', workspaceId], + }); + } onSuccess?.(); }, onError: (err) => { diff --git a/frontend/src/lib/api.ts b/frontend/src/lib/api.ts index 1bf7091d7f..16c8c2062d 100644 --- a/frontend/src/lib/api.ts +++ b/frontend/src/lib/api.ts @@ -478,6 +478,19 @@ export const sessionsApi = { }); return handleApiResponse(response); }, + + startReview: async ( + sessionId: string, + data: StartReviewRequest + ): Promise> => { + const response = await makeRequest(`/api/sessions/${sessionId}/review`, { + method: 'POST', + body: JSON.stringify(data), + }); + return handleApiResponse>( + response + ); + }, }; // Task Attempts APIs @@ -540,22 +553,6 @@ export const attemptsApi = { return handleApiResponse(response); }, - startReview: async ( - attemptId: string, - data: StartReviewRequest - ): Promise> => { - const response = await makeRequest( - `/api/task-attempts/${attemptId}/review`, - { - method: 'POST', - body: JSON.stringify(data), - } - ); - return handleApiResponse>( - response - ); - }, - delete: async (attemptId: string): Promise => { const response = await makeRequest(`/api/task-attempts/${attemptId}`, { method: 'DELETE', From 33878afd3f2176752188777cd3c3e5c17a28baef Mon Sep 17 00:00:00 2001 From: Alex Netsch Date: Thu, 8 Jan 2026 12:20:29 +0000 Subject: [PATCH 11/54] Done. The fix adds the `ReviewRequest` case to `latest_executor_profile_for_session()` in `crates/db/src/models/execution_process.rs:688-690`. The backend check passes. Follow-ups after reviews should now work correctly since the executor profile can be extracted from `ReviewRequest` just like from the other request types. --- crates/db/src/models/execution_process.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crates/db/src/models/execution_process.rs b/crates/db/src/models/execution_process.rs index 0bf7cf6a07..3be615c783 100644 --- a/crates/db/src/models/execution_process.rs +++ b/crates/db/src/models/execution_process.rs @@ -686,6 +686,9 @@ impl ExecutionProcess { ExecutorActionType::CodingAgentFollowUpRequest(request) => { Ok(Some(request.executor_profile_id.clone())) } + ExecutorActionType::ReviewRequest(request) => { + Ok(Some(request.executor_profile_id.clone())) + } _ => Err(ExecutionProcessError::ValidationError( "Couldn't find profile from initial request".to_string(), )), From b77ca4281d80118ab0126096450b2c5f9b91c05a Mon Sep 17 00:00:00 2001 From: Alex Netsch Date: Thu, 8 Jan 2026 12:37:43 +0000 Subject: [PATCH 12/54] Done. Removed `CommitRange` enum entirely. Changes: 1. **`crates/executors/src/actions/review.rs`** - Deleted `CommitRange` enum, changed `RepoReviewContext.commits` to `RepoReviewContext.base_commit: String` 2. **`crates/server/src/routes/sessions/review.rs`** - Updated construction to use `base_commit: initial_commit` instead of `commits: CommitRange::FromBase { commit: initial_commit }` 3. **`crates/executors/src/executors/mod.rs`** - Simplified `build_review_prompt()` to directly use `repo.base_commit` instead of matching on the enum 4. **`crates/server/src/bin/generate_types.rs`** - Removed `CommitRange::decl()` from type exports 5. **`shared/types.ts`** - Regenerated, `CommitRange` type is now gone Cleanup script changes for workspace 38f8b607-9fdc-43dc-afa3-16e8f5e9cbd3 --- crates/executors/src/actions/review.rs | 16 ++------- crates/executors/src/executors/mod.rs | 39 +++++---------------- crates/server/src/bin/generate_types.rs | 1 - crates/server/src/routes/sessions/review.rs | 9 ++--- shared/types.ts | 8 +++-- 5 files changed, 17 insertions(+), 56 deletions(-) diff --git a/crates/executors/src/actions/review.rs b/crates/executors/src/actions/review.rs index 9122eea429..3053d1de78 100644 --- a/crates/executors/src/actions/review.rs +++ b/crates/executors/src/actions/review.rs @@ -13,25 +13,13 @@ use crate::{ profile::{ExecutorConfigs, ExecutorProfileId}, }; -/// Specifies which commits to review -#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, TS)] -#[serde(tag = "type", rename_all = "snake_case")] -#[ts(tag = "type", rename_all = "snake_case")] -pub enum CommitRange { - /// All commits from this base commit to HEAD - FromBase { commit: String }, - /// Specific commits to review - Specific { commits: Vec }, - /// Range from one commit to another - Range { from: String, to: String }, -} - /// Context for a repository in a review request #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, TS)] pub struct RepoReviewContext { pub repo_id: Uuid, pub repo_name: String, - pub commits: CommitRange, + /// The base commit - review all changes from here to HEAD + pub base_commit: String, } /// Request to start a code review session diff --git a/crates/executors/src/executors/mod.rs b/crates/executors/src/executors/mod.rs index f73495022b..7b74a5645f 100644 --- a/crates/executors/src/executors/mod.rs +++ b/crates/executors/src/executors/mod.rs @@ -319,42 +319,19 @@ pub fn build_review_prompt( context: Option<&[RepoReviewContext]>, additional_prompt: Option<&str>, ) -> String { - use crate::actions::review::CommitRange; - let mut prompt = String::from("Please review the code changes.\n\n"); if let Some(repos) = context { for repo in repos { prompt.push_str(&format!("Repository: {}\n", repo.repo_name)); - - match &repo.commits { - CommitRange::FromBase { commit } => { - prompt.push_str(&format!( - "Review all changes from base commit {} to HEAD.\n", - commit - )); - prompt.push_str(&format!( - "Use `git diff {}..HEAD` to see the changes.\n", - commit - )); - } - CommitRange::Specific { commits } => { - prompt.push_str("Review the following commits:\n"); - for hash in commits { - prompt.push_str(&format!("- {}\n", hash)); - } - } - CommitRange::Range { from, to } => { - prompt.push_str(&format!( - "Review all changes from commit {} to {}.\n", - from, to - )); - prompt.push_str(&format!( - "Use `git diff {}..{}` to see the changes.\n", - from, to - )); - } - } + prompt.push_str(&format!( + "Review all changes from base commit {} to HEAD.\n", + repo.base_commit + )); + prompt.push_str(&format!( + "Use `git diff {}..HEAD` to see the changes.\n", + repo.base_commit + )); prompt.push('\n'); } } diff --git a/crates/server/src/bin/generate_types.rs b/crates/server/src/bin/generate_types.rs index a50fc238f5..8171fd9ea4 100644 --- a/crates/server/src/bin/generate_types.rs +++ b/crates/server/src/bin/generate_types.rs @@ -200,7 +200,6 @@ fn generate_types_content() -> String { executors::actions::coding_agent_follow_up::CodingAgentFollowUpRequest::decl(), executors::actions::review::ReviewRequest::decl(), executors::actions::review::RepoReviewContext::decl(), - executors::actions::review::CommitRange::decl(), executors::logs::CommandExitStatus::decl(), executors::logs::CommandRunResult::decl(), executors::logs::NormalizedEntry::decl(), diff --git a/crates/server/src/routes/sessions/review.rs b/crates/server/src/routes/sessions/review.rs index 6b8ca82dc3..cd24d4b586 100644 --- a/crates/server/src/routes/sessions/review.rs +++ b/crates/server/src/routes/sessions/review.rs @@ -10,10 +10,7 @@ use deployment::Deployment; use executors::{ actions::{ ExecutorAction, ExecutorActionType, - review::{ - CommitRange, RepoReviewContext as ExecutorRepoReviewContext, - ReviewRequest as ReviewAction, - }, + review::{RepoReviewContext as ExecutorRepoReviewContext, ReviewRequest as ReviewAction}, }, executors::build_review_prompt, profile::ExecutorProfileId, @@ -102,9 +99,7 @@ pub async fn start_review( Some(ExecutorRepoReviewContext { repo_id, repo_name: repo.display_name.clone(), - commits: CommitRange::FromBase { - commit: initial_commit, - }, + base_commit: initial_commit, }) }) .collect(), diff --git a/shared/types.ts b/shared/types.ts index fd309ed305..639478f788 100644 --- a/shared/types.ts +++ b/shared/types.ts @@ -537,9 +537,11 @@ session_id: string | null, */ working_dir: string | null, }; -export type RepoReviewContext = { repo_id: string, repo_name: string, commits: CommitRange, }; - -export type CommitRange = { "type": "from_base", commit: string, } | { "type": "specific", commits: Array, } | { "type": "range", from: string, to: string, }; +export type RepoReviewContext = { repo_id: string, repo_name: string, +/** + * The base commit - review all changes from here to HEAD + */ +base_commit: string, }; export type CommandExitStatus = { "type": "exit_code", code: number, } | { "type": "success", success: boolean, }; From 3bd63fa6e809de3c32d2d38334ec0ef7213b1001 Mon Sep 17 00:00:00 2001 From: Alex Netsch Date: Thu, 8 Jan 2026 13:54:57 +0000 Subject: [PATCH 13/54] Done. Simplified the review context building: MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Changes:** 1. **`crates/db/src/models/execution_process_repo_state.rs`** - Replaced `find_initial_commits_for_workspace` with `find_initial_commit_for_repo` that takes a specific repo_id and uses ORDER BY + LIMIT 1 instead of GROUP BY/HAVING. 2. **`crates/server/src/routes/sessions/review.rs`** - Simplified the context building: - Get workspace repos first (has repo names via `display_name`) - For each repo, get initial commit - Build context directly - no more HashMap joining, no more separate `Repo::find_by_ids` call The new code is much easier to follow: get repos → for each, get initial commit → build context. --- ...d232414f1040714520abd39b0a44bbb7e6779.json | 20 +++++++++ ...0a42efb8f38bbb00c504555117743aaa01161.json | 26 ----------- .../models/execution_process_repo_state.rs | 31 ++++++------- crates/server/src/routes/sessions/review.rs | 44 +++++++------------ 4 files changed, 51 insertions(+), 70 deletions(-) create mode 100644 crates/db/.sqlx/query-863bc34e8b0b0cb45d1cd9cb813d232414f1040714520abd39b0a44bbb7e6779.json delete mode 100644 crates/db/.sqlx/query-b01828fc0b1d7a6d75821e132ab0a42efb8f38bbb00c504555117743aaa01161.json diff --git a/crates/db/.sqlx/query-863bc34e8b0b0cb45d1cd9cb813d232414f1040714520abd39b0a44bbb7e6779.json b/crates/db/.sqlx/query-863bc34e8b0b0cb45d1cd9cb813d232414f1040714520abd39b0a44bbb7e6779.json new file mode 100644 index 0000000000..d042b34c4a --- /dev/null +++ b/crates/db/.sqlx/query-863bc34e8b0b0cb45d1cd9cb813d232414f1040714520abd39b0a44bbb7e6779.json @@ -0,0 +1,20 @@ +{ + "db_name": "SQLite", + "query": "SELECT eprs.before_head_commit as \"before_head_commit!\"\n FROM execution_process_repo_states eprs\n JOIN execution_processes ep ON ep.id = eprs.execution_process_id\n JOIN sessions s ON s.id = ep.session_id\n WHERE s.workspace_id = $1\n AND eprs.repo_id = $2\n AND eprs.before_head_commit IS NOT NULL\n ORDER BY ep.created_at ASC\n LIMIT 1", + "describe": { + "columns": [ + { + "name": "before_head_commit!", + "ordinal": 0, + "type_info": "Text" + } + ], + "parameters": { + "Right": 2 + }, + "nullable": [ + true + ] + }, + "hash": "863bc34e8b0b0cb45d1cd9cb813d232414f1040714520abd39b0a44bbb7e6779" +} diff --git a/crates/db/.sqlx/query-b01828fc0b1d7a6d75821e132ab0a42efb8f38bbb00c504555117743aaa01161.json b/crates/db/.sqlx/query-b01828fc0b1d7a6d75821e132ab0a42efb8f38bbb00c504555117743aaa01161.json deleted file mode 100644 index 484e38d100..0000000000 --- a/crates/db/.sqlx/query-b01828fc0b1d7a6d75821e132ab0a42efb8f38bbb00c504555117743aaa01161.json +++ /dev/null @@ -1,26 +0,0 @@ -{ - "db_name": "SQLite", - "query": "SELECT\n eprs.repo_id as \"repo_id!: Uuid\",\n eprs.before_head_commit as \"before_head_commit!\"\n FROM execution_process_repo_states eprs\n JOIN execution_processes ep ON ep.id = eprs.execution_process_id\n JOIN sessions s ON s.id = ep.session_id\n WHERE s.workspace_id = $1\n AND eprs.before_head_commit IS NOT NULL\n GROUP BY eprs.repo_id\n HAVING ep.created_at = MIN(ep.created_at)", - "describe": { - "columns": [ - { - "name": "repo_id!: Uuid", - "ordinal": 0, - "type_info": "Blob" - }, - { - "name": "before_head_commit!", - "ordinal": 1, - "type_info": "Text" - } - ], - "parameters": { - "Right": 1 - }, - "nullable": [ - false, - true - ] - }, - "hash": "b01828fc0b1d7a6d75821e132ab0a42efb8f38bbb00c504555117743aaa01161" -} diff --git a/crates/db/src/models/execution_process_repo_state.rs b/crates/db/src/models/execution_process_repo_state.rs index 500cddd8a1..e34ebe49d8 100644 --- a/crates/db/src/models/execution_process_repo_state.rs +++ b/crates/db/src/models/execution_process_repo_state.rs @@ -157,32 +157,29 @@ impl ExecutionProcessRepoState { .await } - /// Find the initial before_head_commit for each repo in a workspace. - /// Returns the before_head_commit from the earliest execution process for each repo. - pub async fn find_initial_commits_for_workspace( + /// Find the initial before_head_commit for a specific repo in a workspace. + /// Returns the before_head_commit from the earliest execution process for that repo. + pub async fn find_initial_commit_for_repo( pool: &SqlitePool, workspace_id: Uuid, - ) -> Result, sqlx::Error> { - // For each repo, find the before_head_commit from the earliest process - let rows = sqlx::query!( - r#"SELECT - eprs.repo_id as "repo_id!: Uuid", - eprs.before_head_commit as "before_head_commit!" + repo_id: Uuid, + ) -> Result, sqlx::Error> { + let row = sqlx::query!( + r#"SELECT eprs.before_head_commit as "before_head_commit!" FROM execution_process_repo_states eprs JOIN execution_processes ep ON ep.id = eprs.execution_process_id JOIN sessions s ON s.id = ep.session_id WHERE s.workspace_id = $1 + AND eprs.repo_id = $2 AND eprs.before_head_commit IS NOT NULL - GROUP BY eprs.repo_id - HAVING ep.created_at = MIN(ep.created_at)"#, - workspace_id + ORDER BY ep.created_at ASC + LIMIT 1"#, + workspace_id, + repo_id ) - .fetch_all(pool) + .fetch_optional(pool) .await?; - Ok(rows - .into_iter() - .map(|r| (r.repo_id, r.before_head_commit)) - .collect()) + Ok(row.map(|r| r.before_head_commit)) } } diff --git a/crates/server/src/routes/sessions/review.rs b/crates/server/src/routes/sessions/review.rs index cd24d4b586..55ee32a6ed 100644 --- a/crates/server/src/routes/sessions/review.rs +++ b/crates/server/src/routes/sessions/review.rs @@ -2,9 +2,9 @@ use axum::{Extension, Json, extract::State, response::Json as ResponseJson}; use db::models::{ execution_process::{ExecutionProcess, ExecutionProcessRunReason}, execution_process_repo_state::ExecutionProcessRepoState, - repo::Repo, session::Session, workspace::{Workspace, WorkspaceError}, + workspace_repo::WorkspaceRepo, }; use deployment::Deployment; use executors::{ @@ -19,7 +19,6 @@ use serde::{Deserialize, Serialize}; use services::services::container::ContainerService; use ts_rs::TS; use utils::response::ApiResponse; -use uuid::Uuid; use crate::{DeploymentImpl, error::ApiError}; @@ -77,33 +76,24 @@ pub async fn start_review( // Build context - auto-populated from workspace commits when requested let context: Option> = if payload.use_all_workspace_commits { - // Auto-populate with initial commits for each repo in the workspace - let initial_commits = - ExecutionProcessRepoState::find_initial_commits_for_workspace(pool, workspace.id) - .await?; - - if initial_commits.is_empty() { + let repos = WorkspaceRepo::find_repos_for_workspace(pool, workspace.id).await?; + let mut contexts = Vec::new(); + for repo in repos { + if let Some(base_commit) = + ExecutionProcessRepoState::find_initial_commit_for_repo(pool, workspace.id, repo.id) + .await? + { + contexts.push(ExecutorRepoReviewContext { + repo_id: repo.id, + repo_name: repo.display_name, + base_commit, + }); + } + } + if contexts.is_empty() { None } else { - // Look up repo names - let repo_ids: Vec = initial_commits.iter().map(|(id, _)| *id).collect(); - let repos = Repo::find_by_ids(pool, &repo_ids).await?; - let repo_map: std::collections::HashMap = - repos.iter().map(|r| (r.id, r)).collect(); - - Some( - initial_commits - .into_iter() - .filter_map(|(repo_id, initial_commit)| { - let repo = repo_map.get(&repo_id)?; - Some(ExecutorRepoReviewContext { - repo_id, - repo_name: repo.display_name.clone(), - base_commit: initial_commit, - }) - }) - .collect(), - ) + Some(contexts) } } else { None From 5601e4b84cb1f9405f6d57ab4fe68ce054451dcf Mon Sep 17 00:00:00 2001 From: Alex Netsch Date: Thu, 8 Jan 2026 14:13:16 +0000 Subject: [PATCH 14/54] Done. Addressed all 23 review comments: 1. Removed comment from `SessionChatBoxContainer.tsx` (line 161) 2. Removed doc comments from `RepoReviewContext` and `ReviewRequest` structs in `review.rs` 3. Removed module doc comment from `codex/review.rs` 4. Removed comment from `codex.rs` about Custom review target 5. Removed doc comments from `spawn_review` trait method in `mod.rs` 6. Removed doc comment from `build_review_prompt` function 7. Removed all comments from `sessions/review.rs` (doc comments and inline comments) 8. Removed unused `get_commits_since_branch` function from `git.rs` (~45 lines of dead code) --- crates/executors/src/actions/review.rs | 4 -- crates/executors/src/executors/codex.rs | 1 - .../executors/src/executors/codex/review.rs | 3 -- crates/executors/src/executors/mod.rs | 3 -- crates/server/src/routes/sessions/review.rs | 14 ------ crates/services/src/services/git.rs | 45 ------------------- .../containers/SessionChatBoxContainer.tsx | 1 - 7 files changed, 71 deletions(-) diff --git a/crates/executors/src/actions/review.rs b/crates/executors/src/actions/review.rs index 3053d1de78..4874f713cf 100644 --- a/crates/executors/src/actions/review.rs +++ b/crates/executors/src/actions/review.rs @@ -13,21 +13,17 @@ use crate::{ profile::{ExecutorConfigs, ExecutorProfileId}, }; -/// Context for a repository in a review request #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, TS)] pub struct RepoReviewContext { pub repo_id: Uuid, pub repo_name: String, - /// The base commit - review all changes from here to HEAD pub base_commit: String, } -/// Request to start a code review session #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, TS)] pub struct ReviewRequest { pub executor_profile_id: ExecutorProfileId, pub context: Option>, - /// The full prompt sent to the model pub prompt: String, /// Optional session ID to resume an existing session #[serde(default)] diff --git a/crates/executors/src/executors/codex.rs b/crates/executors/src/executors/codex.rs index 325b7cb624..0568a065b0 100644 --- a/crates/executors/src/executors/codex.rs +++ b/crates/executors/src/executors/codex.rs @@ -229,7 +229,6 @@ impl StandardCodingAgentExecutor for Codex { env: &ExecutionEnv, ) -> Result { let command_parts = self.build_command_builder().build_initial()?; - // Use Custom review target with the pre-built prompt let review_target = ReviewTarget::Custom { instructions: prompt.to_string(), }; diff --git a/crates/executors/src/executors/codex/review.rs b/crates/executors/src/executors/codex/review.rs index a9ef1ad4a6..e61e20b8e4 100644 --- a/crates/executors/src/executors/codex/review.rs +++ b/crates/executors/src/executors/codex/review.rs @@ -1,5 +1,3 @@ -//! Review-specific functionality for the Codex executor. - use std::sync::Arc; use codex_app_server_protocol::{NewConversationParams, ReviewTarget}; @@ -11,7 +9,6 @@ use super::{ }; use crate::{approvals::ExecutorApprovalService, executors::ExecutorError}; -/// Launch a Codex review session. #[allow(clippy::too_many_arguments)] pub async fn launch_codex_review( conversation_params: NewConversationParams, diff --git a/crates/executors/src/executors/mod.rs b/crates/executors/src/executors/mod.rs index 7b74a5645f..95a5e5256d 100644 --- a/crates/executors/src/executors/mod.rs +++ b/crates/executors/src/executors/mod.rs @@ -216,8 +216,6 @@ pub trait StandardCodingAgentExecutor { env: &ExecutionEnv, ) -> Result; - /// Spawn a review session. Default implementation delegates to spawn() or spawn_follow_up(). - /// Executors with native review support (e.g., Codex) can override this. async fn spawn_review( &self, current_dir: &Path, @@ -314,7 +312,6 @@ impl AppendPrompt { } } -/// Build a natural language review prompt from context and additional instructions. pub fn build_review_prompt( context: Option<&[RepoReviewContext]>, additional_prompt: Option<&str>, diff --git a/crates/server/src/routes/sessions/review.rs b/crates/server/src/routes/sessions/review.rs index 55ee32a6ed..eea07ad9c7 100644 --- a/crates/server/src/routes/sessions/review.rs +++ b/crates/server/src/routes/sessions/review.rs @@ -22,17 +22,14 @@ use utils::response::ApiResponse; use crate::{DeploymentImpl, error::ApiError}; -/// Request to start a review session #[derive(Debug, Deserialize, Serialize, TS)] pub struct StartReviewRequest { pub executor_profile_id: ExecutorProfileId, pub additional_prompt: Option, - /// If true, automatically include all workspace commits from initial state #[serde(default)] pub use_all_workspace_commits: bool, } -/// Error types for review operations #[derive(Debug, Serialize, Deserialize, TS)] #[serde(tag = "type", rename_all = "snake_case")] #[ts(tag = "type", rename_all = "snake_case")] @@ -48,14 +45,12 @@ pub async fn start_review( ) -> Result>, ApiError> { let pool = &deployment.db().pool; - // Load workspace from session let workspace = Workspace::find_by_id(pool, session.workspace_id) .await? .ok_or(ApiError::Workspace(WorkspaceError::ValidationError( "Workspace not found".to_string(), )))?; - // Check if any non-dev-server processes are already running for this workspace if ExecutionProcess::has_running_non_dev_server_processes_for_workspace(pool, workspace.id) .await? { @@ -64,17 +59,14 @@ pub async fn start_review( ))); } - // Ensure container exists deployment .container() .ensure_container_exists(&workspace) .await?; - // Lookup agent session_id from previous execution in this session (for session resumption) let agent_session_id = ExecutionProcess::find_latest_coding_agent_turn_session_id(pool, session.id).await?; - // Build context - auto-populated from workspace commits when requested let context: Option> = if payload.use_all_workspace_commits { let repos = WorkspaceRepo::find_repos_for_workspace(pool, workspace.id).await?; let mut contexts = Vec::new(); @@ -99,13 +91,9 @@ pub async fn start_review( None }; - // Build the full prompt for display and execution let prompt = build_review_prompt(context.as_deref(), payload.additional_prompt.as_deref()); - - // Track whether we're resuming a session (before moving agent_session_id) let resumed_session = agent_session_id.is_some(); - // Build the review action let action = ExecutorAction::new( ExecutorActionType::ReviewRequest(ReviewAction { executor_profile_id: payload.executor_profile_id.clone(), @@ -117,7 +105,6 @@ pub async fn start_review( None, ); - // Start execution let execution_process = deployment .container() .start_execution( @@ -128,7 +115,6 @@ pub async fn start_review( ) .await?; - // Track analytics deployment .track_if_analytics_allowed( "review_started", diff --git a/crates/services/src/services/git.rs b/crates/services/src/services/git.rs index 9dc6449e62..5562ec0a8b 100644 --- a/crates/services/src/services/git.rs +++ b/crates/services/src/services/git.rs @@ -1869,49 +1869,4 @@ impl GitService { Ok(stats) } - - /// Get commits on current_branch that are not on target_branch. - /// Returns list of commit SHAs (oldest first). - pub fn get_commits_since_branch( - &self, - repo_path: &Path, - current_branch: &str, - target_branch: &str, - ) -> Result, GitServiceError> { - let repo = self.open_repo(repo_path)?; - - // Find the current branch HEAD - let current_ref = repo - .find_branch(current_branch, BranchType::Local)? - .get() - .target() - .ok_or_else(|| GitServiceError::BranchNotFound(current_branch.to_string()))?; - - // Find the target branch HEAD - try local first, then remote - let target_oid = if let Ok(branch) = repo.find_branch(target_branch, BranchType::Local) { - branch - .get() - .target() - .ok_or_else(|| GitServiceError::BranchNotFound(target_branch.to_string()))? - } else { - // Try as remote branch (e.g., "origin/main") - let remote_ref = format!("refs/remotes/{}", target_branch); - repo.refname_to_id(&remote_ref) - .map_err(|_| GitServiceError::BranchNotFound(target_branch.to_string()))? - }; - - // Set up revwalk - let mut revwalk = repo.revwalk()?; - revwalk.push(current_ref)?; - revwalk.hide(target_oid)?; - revwalk.set_sorting(Sort::REVERSE | Sort::TIME)?; // Oldest first - - // Collect commit SHAs - let commits: Vec = revwalk - .filter_map(|oid_result| oid_result.ok()) - .map(|oid| oid.to_string()) - .collect(); - - Ok(commits) - } } diff --git a/frontend/src/components/ui-new/containers/SessionChatBoxContainer.tsx b/frontend/src/components/ui-new/containers/SessionChatBoxContainer.tsx index bedf60bfb1..abd64a4ecd 100644 --- a/frontend/src/components/ui-new/containers/SessionChatBoxContainer.tsx +++ b/frontend/src/components/ui-new/containers/SessionChatBoxContainer.tsx @@ -156,7 +156,6 @@ export function SessionChatBoxContainer({ const { approveAsync, denyAsync, isApproving, isDenying, denyError } = useApprovalMutation(); - // Review mutation for starting reviews const startReviewMutation = useStartReview(sessionId, workspaceId); // Branch status for edit retry and conflict detection From 9c071b6d1b738449fd5048911afd07a59cade528 Mon Sep 17 00:00:00 2001 From: Alex Netsch Date: Thu, 8 Jan 2026 14:54:32 +0000 Subject: [PATCH 15/54] Done. Added the review toggle button: MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Changes:** 1. **`SessionChatBox.tsx`** - Added `ReviewModeProps` interface with `isActive`, `onToggle`, `isSubmitting` - Added `reviewMode` prop to `SessionChatBoxProps` - Added toggle button next to attach button (MagnifyingGlassIcon) - highlighted when active - Updated placeholder to show "Enter review instructions..." when in review mode - Removed old `onStartReview` and `isReviewStarting` from SessionProps - Removed "Start Review" dropdown menu item 2. **`SessionChatBoxContainer.tsx`** - Added `useState` for `isReviewMode` - Updated `handleSend` to call `startReviewMutation` with `additionalPrompt` when in review mode - Reset review mode after successful review start - Pass `reviewMode` props to SessionChatBox - Removed old `handleStartReview` callback Now the user can: 1. Click the magnifying glass toggle next to attach 2. Enter review instructions in the text area 3. Click send → starts review with the prompt Cleanup script changes for workspace 38f8b607-9fdc-43dc-afa3-16e8f5e9cbd3 TypeScript and Rust both compile. The changes are: 1. Button shows "Review" when review mode is active 2. Toggle icon uses `!text-accent-foreground` to force override the `text-low` class from the base component, along with `bg-accent rounded-sm` for the background highlight Done. Changed `!text-accent-foreground` to `!text-normal` which uses the design system's proper text color for active/selected states. The `!` prefix forces the override of the base `text-low` class. Both TypeScript and Rust compile successfully. Cleanup script changes for workspace 38f8b607-9fdc-43dc-afa3-16e8f5e9cbd3 All done. TypeScript and Rust compile successfully. Here's a summary of the changes: **Created:** - `frontend/src/components/dialogs/tasks/StartReviewDialog.tsx` - New dialog with: - Agent/variant selection (using ExecutorProfileSelector) - Review comments preview (if any) - Additional instructions textarea - Start Review / Cancel buttons **Modified:** - `frontend/src/components/dialogs/index.ts` - Added export for StartReviewDialog - `frontend/src/components/ui-new/primitives/SessionChatBox.tsx`: - Changed `ReviewModeProps` from toggle state to simple callback (`onReviewClick`, `isDisabled`) - Removed toggle styling and review mode button text - Button now just opens the dialog - `frontend/src/components/ui-new/containers/SessionChatBoxContainer.tsx`: - Removed `isReviewMode` state and `startReviewMutation` - Added `handleReviewClick` callback that opens `StartReviewDialog` - Disabled review button when in new session mode or no session exists Cleanup script changes for workspace 38f8b607-9fdc-43dc-afa3-16e8f5e9cbd3 Done. TypeScript and Rust compile successfully. Summary of changes: **`StartReviewDialog.tsx`:** - Added "New Session" toggle (defaults ON when no session exists, disabled when no session to toggle from) - Added conditional `AgentSelector` - only shown when creating a new session - Always shows `ConfigSelector` for variant selection - Handles session creation inline when toggle is on or no session exists - Updated `onSuccess` callback to return new session ID when created **`SessionChatBox.tsx`:** - Removed `isDisabled` from `ReviewModeProps` interface - Removed `reviewMode.isDisabled` from button disabled logic **`SessionChatBoxContainer.tsx`:** - Updated `handleReviewClick` to allow opening dialog without sessionId - Updated `onSuccess` callback to select the new session if one was created - Removed `isDisabled` from reviewMode prop Cleanup script changes for workspace 38f8b607-9fdc-43dc-afa3-16e8f5e9cbd3 Done. Made `reviewMode` required and removed the conditional - the review button is now always shown. Done. Now the StartReviewDialog: - Shows `ExecutorProfileSelector` (Agent + Configuration side by side, like CreateAttemptDialog) when "New Session" toggle is ON - Shows just `ConfigSelector` (variant only) when "New Session" toggle is OFF TypeScript and Rust compile successfully. The Switch layout has been fixed to match the TaskFormDialog pattern: - Switch is now on the LEFT of the label - Uses `gap-2` instead of `justify-between` - Has proper dark mode styling with `data-[state=checked]:bg-gray-900 dark:data-[state=checked]:bg-gray-100` - Label has `cursor-pointer` for better UX Cleanup script changes for workspace 38f8b607-9fdc-43dc-afa3-16e8f5e9cbd3 Done. The Switch is now moved to the footer (bottom of the dialog), matching the TaskFormDialog pattern: - Switch on the left side of the footer - Cancel and Start Review buttons on the right side - Added `disabled:opacity-50` for better visibility when disabled in dark mode Done. All changes complete: 1. **Removed header** - No more "Start Review" title or description 2. **Removed labels from selectors** - `showLabel={false}` for both `ExecutorProfileSelector` and `ConfigSelector` 3. **Fixed footer layout** - Cancel button on left, Switch + "New Session" + "Start Review" button grouped on right (matching TaskFormDialog) 4. **TypeScript compiles** - Both frontend and backend checks pass Done. Changes made: 1. **Moved selector to bottom** - Now the order is: Additional Instructions textarea → Review comments preview → Error → Profile/Config selector → Footer 2. **Fixed Switch visibility** - Changed from `data-[state=checked]:bg-gray-900 dark:data-[state=checked]:bg-gray-100` to `bg-muted-foreground/30 data-[state=checked]:bg-foreground` using semantic colors 3. **TypeScript compiles** - Both frontend and backend checks pass Done. The Switch className now uses `!` important modifiers to override the component's hardcoded background colors: - `!bg-muted-foreground/30` - visible gray background when unchecked (overrides `bg-input`) - `data-[state=checked]:!bg-foreground` - foreground color when checked (overrides `bg-foreground`) Done. Changed the Switch className to use `!bg-muted` which should provide better contrast against the dialog's `bg-primary` background. Done. Changed the Switch className to use `!bg-border` which should provide some contrast against the dialog's `bg-primary` background (25% lightness vs 18% lightness). --- frontend/src/components/dialogs/index.ts | 4 + .../dialogs/tasks/StartReviewDialog.tsx | 208 ++++++++++++++++++ .../containers/SessionChatBoxContainer.tsx | 50 +++-- .../ui-new/primitives/SessionChatBox.tsx | 37 +--- 4 files changed, 250 insertions(+), 49 deletions(-) create mode 100644 frontend/src/components/dialogs/tasks/StartReviewDialog.tsx diff --git a/frontend/src/components/dialogs/index.ts b/frontend/src/components/dialogs/index.ts index 8932454dd1..94d5373975 100644 --- a/frontend/src/components/dialogs/index.ts +++ b/frontend/src/components/dialogs/index.ts @@ -93,6 +93,10 @@ export { type EditBranchNameDialogResult, } from './tasks/EditBranchNameDialog'; export { CreateAttemptDialog } from './tasks/CreateAttemptDialog'; +export { + StartReviewDialog, + type StartReviewDialogProps, +} from './tasks/StartReviewDialog'; // Auth dialogs export { GhCliSetupDialog } from './auth/GhCliSetupDialog'; diff --git a/frontend/src/components/dialogs/tasks/StartReviewDialog.tsx b/frontend/src/components/dialogs/tasks/StartReviewDialog.tsx new file mode 100644 index 0000000000..3a43dca4c3 --- /dev/null +++ b/frontend/src/components/dialogs/tasks/StartReviewDialog.tsx @@ -0,0 +1,208 @@ +import { useState, useCallback } from 'react'; +import { Dialog, DialogContent, DialogFooter } from '@/components/ui/dialog'; +import { Button } from '@/components/ui/button'; +import { Textarea } from '@/components/ui/textarea'; +import { Label } from '@/components/ui/label'; +import { Switch } from '@/components/ui/switch'; +import { ExecutorProfileSelector } from '@/components/settings'; +import { ConfigSelector } from '@/components/tasks/ConfigSelector'; +import { useUserSystem } from '@/components/ConfigProvider'; +import { sessionsApi } from '@/lib/api'; +import { useQueryClient } from '@tanstack/react-query'; +import NiceModal, { useModal } from '@ebay/nice-modal-react'; +import { defineModal } from '@/lib/modals'; +import type { ExecutorProfileId } from 'shared/types'; + +export interface StartReviewDialogProps { + sessionId?: string; + workspaceId: string; + reviewMarkdown?: string; + defaultProfile?: ExecutorProfileId | null; + onSuccess?: (newSessionId?: string) => void; +} + +const StartReviewDialogImpl = NiceModal.create( + ({ sessionId, workspaceId, reviewMarkdown, defaultProfile, onSuccess }) => { + const modal = useModal(); + const queryClient = useQueryClient(); + const { profiles } = useUserSystem(); + + const [selectedProfile, setSelectedProfile] = + useState(defaultProfile ?? null); + const [additionalPrompt, setAdditionalPrompt] = useState(''); + const [createNewSession, setCreateNewSession] = useState(!sessionId); + const [isSubmitting, setIsSubmitting] = useState(false); + const [error, setError] = useState(null); + + const effectiveProfile = selectedProfile ?? defaultProfile ?? null; + + const canSubmit = Boolean(effectiveProfile && !isSubmitting); + + const handleSubmit = useCallback(async () => { + if (!effectiveProfile) return; + + setIsSubmitting(true); + setError(null); + + try { + let targetSessionId = sessionId; + + if (createNewSession || !sessionId) { + const session = await sessionsApi.create({ + workspace_id: workspaceId, + executor: effectiveProfile.executor, + }); + targetSessionId = session.id; + + queryClient.invalidateQueries({ + queryKey: ['workspaceSessions', workspaceId], + }); + } + + if (!targetSessionId) { + setError('Failed to create session'); + setIsSubmitting(false); + return; + } + + const promptParts = [reviewMarkdown, additionalPrompt].filter(Boolean); + const combinedPrompt = promptParts.join('\n\n'); + + await sessionsApi.startReview(targetSessionId, { + executor_profile_id: effectiveProfile, + additional_prompt: combinedPrompt || null, + use_all_workspace_commits: true, + }); + + queryClient.invalidateQueries({ + queryKey: ['processes', workspaceId], + }); + queryClient.invalidateQueries({ + queryKey: ['branchStatus', workspaceId], + }); + + const createdNewSession = targetSessionId !== sessionId; + onSuccess?.(createdNewSession ? targetSessionId : undefined); + modal.hide(); + } catch (err) { + console.error('Failed to start review:', err); + setError('Failed to start review. Please try again.'); + } finally { + setIsSubmitting(false); + } + }, [ + effectiveProfile, + sessionId, + workspaceId, + createNewSession, + reviewMarkdown, + additionalPrompt, + queryClient, + onSuccess, + modal, + ]); + + const handleOpenChange = (open: boolean) => { + if (!open) modal.hide(); + }; + + const hasReviewComments = Boolean(reviewMarkdown); + + return ( + + +
+
+ +