diff --git a/apps/decodex/src/config.rs b/apps/decodex/src/config.rs index a39f94923..835d5a690 100644 --- a/apps/decodex/src/config.rs +++ b/apps/decodex/src/config.rs @@ -187,14 +187,15 @@ impl ProjectGitHubConfig { #[serde(deny_unknown_fields)] #[derive(Default)] pub struct ProjectCodexConfig { - #[serde(default = "default_review_level")] - review: ReviewLevel, + review: Option, + external_review_enabled: Option, + internal_review_mode: Option, accounts: Option, } impl ProjectCodexConfig { /// Review level Decodex should apply for agent runs. pub fn review_level(&self) -> ReviewLevel { - self.review + self.review.unwrap_or_else(|| self.legacy_review_level()) } /// Optional ChatGPT accounts used to seed Codex app-server auth. @@ -219,6 +220,22 @@ impl ProjectCodexConfig { Ok(()) } + + fn legacy_review_level(&self) -> ReviewLevel { + match self.external_review_enabled { + Some(true) => ReviewLevel::Strict, + Some(false) => + match self.internal_review_mode.unwrap_or(LegacyInternalReviewMode::Prompt) { + LegacyInternalReviewMode::Prompt => ReviewLevel::Standard, + LegacyInternalReviewMode::Off => ReviewLevel::Basic, + }, + None => match self.internal_review_mode { + Some(LegacyInternalReviewMode::Prompt) => ReviewLevel::Standard, + Some(LegacyInternalReviewMode::Off) => ReviewLevel::Basic, + None => default_review_level(), + }, + } + } } /// Optional local-only classifier for public Linear projection text. @@ -424,6 +441,13 @@ impl Default for ReviewLevel { } } +#[derive(Clone, Copy, Debug, Eq, PartialEq, Deserialize)] +#[serde(rename_all = "snake_case")] +enum LegacyInternalReviewMode { + Prompt, + Off, +} + /// Canonical repository root for the current Git checkout. pub fn canonical_repo_root_for_checkout(cwd: &Path) -> Result> { let worktree_root = git_absolute_rev_parse(cwd, "show-toplevel")? @@ -1195,6 +1219,57 @@ mod tests { } } + #[test] + fn parses_legacy_codex_review_fields() { + let temp_dir = TempDir::new().expect("temp dir should exist"); + let config_path = write_config_file( + temp_dir.path(), + r#" + service_id = "pubfi" + + [tracker] + api_key_env_var = "HOME" + + [github] + token_env_var = "HOME" + + [codex] + external_review_enabled = false + internal_review_mode = "prompt" + "#, + ); + let config = ServiceConfig::from_path(&config_path) + .expect("legacy codex review fields should parse"); + + assert_eq!(config.codex().review_level(), ReviewLevel::Standard); + } + + #[test] + fn explicit_codex_review_field_overrides_legacy_review_fields() { + let temp_dir = TempDir::new().expect("temp dir should exist"); + let config_path = write_config_file( + temp_dir.path(), + r#" + service_id = "pubfi" + + [tracker] + api_key_env_var = "HOME" + + [github] + token_env_var = "HOME" + + [codex] + review = "basic" + external_review_enabled = true + internal_review_mode = "prompt" + "#, + ); + let config = + ServiceConfig::from_path(&config_path).expect("explicit review field should parse"); + + assert_eq!(config.codex().review_level(), ReviewLevel::Basic); + } + #[test] fn rejects_removed_codex_goal_field() { let removed_field = ["goal", "support"].join("_"); diff --git a/apps/decodex/src/orchestrator/run_cycle.rs b/apps/decodex/src/orchestrator/run_cycle.rs index 464d280df..6ff2d2436 100644 --- a/apps/decodex/src/orchestrator/run_cycle.rs +++ b/apps/decodex/src/orchestrator/run_cycle.rs @@ -1558,11 +1558,12 @@ where if excluded_issue_ids.contains(&lane.issue_id.as_str()) { continue; } - if state_store.issue_has_active_shared_claim(project.service_id(), &lane.issue_id)? { - continue; - } if let Some(issue) = issues_by_id.remove(&lane.issue_id) { + if closeout_lane_active_claim_blocks_dispatch(project, state_store, &issue)? { + continue; + } + let preferred_run_identity = retained_closeout_preferred_run_identity(state_store, project.service_id(), &issue)?; @@ -1755,14 +1756,14 @@ where return Ok(None); } - if !context.lease_preacquired - && context - .state_store - .issue_has_active_shared_claim(context.project.service_id(), &issue_id)? - { + + let reuses_existing_closeout_claim = + target_issue_reuses_existing_closeout_claim(&context, &issue_id, &issue)?; + + if target_issue_active_claim_blocks_dispatch(&context, &issue_id, &issue)? { return Ok(None); } - if !context.lease_preacquired { + if !context.lease_preacquired && !reuses_existing_closeout_claim { let concurrency = ConcurrencySnapshot::new(context.project.service_id(), context.state_store)?; if !concurrency.has_global_capacity(context.workflow.frontmatter().execution()) { @@ -1781,7 +1782,7 @@ where state_store: context.state_store, worktree_manager: &worktree_manager, dry_run: context.dry_run, - lease_preacquired: context.lease_preacquired, + lease_preacquired: context.lease_preacquired || reuses_existing_closeout_claim, dispatch_mode: context.dispatch_mode, preferred_issue_state: context.preferred_issue_state, preferred_initial_issue_state: context.preferred_initial_issue_state, @@ -1869,6 +1870,10 @@ fn run_target_issue_once_with_inferred_dispatch( where T: IssueTracker, { + if target_issue_has_status_visible_closeout(&context)? { + return run_target_status_visible_closeout_once(context); + } + if let Some(summary) = run_target_issue_once(target_issue_run_context_with_dispatch_mode( &context, IssueDispatchMode::Normal, @@ -1885,6 +1890,33 @@ where run_target_status_visible_closeout_once(context) } +fn target_issue_has_status_visible_closeout( + context: &TargetIssueRunContext<'_, T>, +) -> Result +where + T: IssueTracker, +{ + let target_issue_id = resolve_target_issue_id(context.tracker, context.issue_id)?; + let completed_state = context.workflow.frontmatter().tracker().resolved_completed_state(); + let review_state_inspector = GhPullRequestReviewStateInspector { + github_token_env_var: Some(context.project.github().token_env_var().to_owned()), + github_command_path: context.project.github().command_path().map(Path::to_path_buf), + }; + + Ok(build_post_review_lane_statuses( + context.tracker, + context.project, + context.workflow, + context.state_store, + &review_state_inspector, + )? + .into_iter() + .any(|lane| { + lane.issue_id == target_issue_id + && post_review_lane_is_closeout_candidate(&lane, completed_state) + })) +} + fn run_target_status_visible_closeout_once( context: TargetIssueRunContext<'_, T>, ) -> Result> @@ -1981,17 +2013,17 @@ where visible_lanes, ); }; - - if state_store.issue_has_active_shared_claim(project.service_id(), &target_lane.issue_id)? { - return Ok(None); - } - let issue_ids = [target_lane.issue_id.clone()]; let mut issues = tracker.refresh_issues(&issue_ids)?; let Some(issue_index) = issues.iter().position(|issue| issue.id == target_lane.issue_id) else { return Ok(None); }; let issue = issues.swap_remove(issue_index); + + if closeout_lane_active_claim_blocks_dispatch(project, state_store, &issue)? { + return Ok(None); + } + let preferred_run_identity = retained_closeout_preferred_run_identity(state_store, project.service_id(), &issue)?; @@ -2002,6 +2034,79 @@ where })) } +fn target_issue_reuses_existing_closeout_claim( + context: &TargetIssueRunContext<'_, T>, + issue_id: &str, + issue: &TrackerIssue, +) -> Result +where + T: IssueTracker, +{ + if context.lease_preacquired || context.dispatch_mode != IssueDispatchMode::Closeout { + return Ok(false); + } + if !context + .state_store + .issue_has_active_shared_claim(context.project.service_id(), issue_id)? + { + return Ok(false); + } + if context.state_store.lease_for_issue(&issue.id)?.is_none() { + return Ok(false); + } + + Ok(!closeout_lane_active_claim_blocks_dispatch( + context.project, + context.state_store, + issue, + )?) +} + +fn target_issue_active_claim_blocks_dispatch( + context: &TargetIssueRunContext<'_, T>, + issue_id: &str, + issue: &TrackerIssue, +) -> Result +where + T: IssueTracker, +{ + if context.lease_preacquired { + return Ok(false); + } + if !context + .state_store + .issue_has_active_shared_claim(context.project.service_id(), issue_id)? + { + return Ok(false); + } + if context.dispatch_mode == IssueDispatchMode::Closeout { + return closeout_lane_active_claim_blocks_dispatch( + context.project, + context.state_store, + issue, + ); + } + + Ok(true) +} + +fn closeout_lane_active_claim_blocks_dispatch( + project: &ServiceConfig, + state_store: &StateStore, + issue: &TrackerIssue, +) -> Result { + if !state_store.issue_has_active_shared_claim(project.service_id(), &issue.id)? { + return Ok(false); + } + + let Some(lease) = state_store.lease_for_issue(&issue.id)? else { + return Ok(true); + }; + let now_unix_epoch = OffsetDateTime::now_utc().unix_timestamp(); + + retained_closeout_lease_has_fresh_activity(&lease, issue, project, now_unix_epoch) +} + fn target_issue_run_context_with_dispatch_mode<'a, T>( context: &TargetIssueRunContext<'a, T>, dispatch_mode: IssueDispatchMode, diff --git a/apps/decodex/src/orchestrator/tests/intake/run_and_prompting.rs b/apps/decodex/src/orchestrator/tests/intake/run_and_prompting.rs index 73ffbf8d2..fee583346 100644 --- a/apps/decodex/src/orchestrator/tests/intake/run_and_prompting.rs +++ b/apps/decodex/src/orchestrator/tests/intake/run_and_prompting.rs @@ -22,6 +22,33 @@ fn run_and_prompting_service_owned_issue(state_name: &str) -> TrackerIssue { sample_issue(state_name, &[active_label.as_str()]) } +fn run_and_prompting_target_context<'a, T>( + tracker: &'a T, + config: &'a ServiceConfig, + workflow: &'a WorkflowDocument, + state_store: &'a StateStore, + issue_identifier: &'a str, + dispatch_mode: IssueDispatchMode, +) -> TargetIssueRunContext<'a, T> { + TargetIssueRunContext { + tracker, + project: config, + workflow, + state_store, + issue_id: issue_identifier, + preferred_issue_state: None, + preferred_initial_issue_state: None, + dry_run: true, + lease_preacquired: false, + preferred_issue_claim_fd: None, + preferred_dispatch_slot_fd: None, + preferred_dispatch_slot_index: None, + dispatch_mode, + preferred_run_identity: None, + preferred_retry_budget_base: None, + } +} + fn assert_prompt_orders_thread_replies_after_push(prompt: &str, push_requirement: &str) { let push_index = prompt.find(push_requirement).expect("prompt should require push before thread resolution"); @@ -282,6 +309,126 @@ fn targeted_identifier_dispatch_accepts_status_visible_retained_closeout_lane() assert_eq!(summary.attempt_number, 1); } +#[test] +fn targeted_identifier_dispatch_accepts_stopped_active_closeout_lease() { + let (temp_dir, base_config, workflow) = temp_project_layout(); + let config = service_config_with_github_token_env_var(&base_config, "HOME"); + let state_store = StateStore::open_in_memory().expect("state store should open"); + let issue = run_and_prompting_service_owned_issue("Done"); + let tracker = FakeTracker::new(vec![issue.clone()]); + let worktree_manager = + WorktreeManager::new(config.service_id(), config.repo_root(), config.worktree_root()); + let worktree = + worktree_manager.ensure_worktree(&issue.identifier, false).expect("worktree should exist"); + let head_oid = git_output(&worktree.path, &["rev-parse", "HEAD"]); + let pr_url = "https://github.com/hack-ink/decodex/pull/183"; + + state_store + .record_run_attempt("run-1", &issue.id, 1, "running") + .expect("stopped active run attempt should record"); + state_store + .upsert_lease(config.service_id(), &issue.id, "run-1", "Done") + .expect("stopped closeout lease should record"); + state_store + .upsert_worktree( + config.service_id(), + &issue.id, + &worktree.branch_name, + &worktree.path.display().to_string(), + ) + .expect("worktree should record"); + + state::write_run_activity_marker_for_process(&worktree.path, "run-1", 1, u32::MAX) + .expect("stopped closeout activity marker should write"); + + seed_review_handoff_marker_for_path( + &state_store, + config.service_id(), + &worktree.path, + &sample_review_handoff_marker(&worktree.branch_name, pr_url, &head_oid), + ); + + let _path_guard = install_fake_merged_pr_gh_response(&temp_dir, &worktree, pr_url, &head_oid); + let snapshot = orchestrator::build_live_operator_status_snapshot( + &tracker, + &config, + &workflow, + &state_store, + 10, + ) + .expect("status snapshot should build"); + let lane = snapshot + .post_review_lanes + .iter() + .find(|lane| lane.issue_identifier == issue.identifier) + .expect("retained closeout lane should appear in status"); + + assert_eq!(lane.classification, "continue"); + assert_eq!(lane.reason, "pull_request_merged_closeout_pending"); + assert_eq!(lane.pr_state.as_deref(), Some("MERGED")); + assert_eq!(snapshot.active_runs.len(), 1); + assert_eq!(snapshot.active_runs[0].process_alive, Some(false)); + assert!( + orchestrator::issue_passes_closeout_dispatch_policy( + &tracker, + &issue, + &config, + &workflow, + &state_store, + ) + .expect("closeout dispatch policy should evaluate"), + "stopped active closeout lease should still satisfy closeout policy" + ); + assert!( + !orchestrator::target_issue_active_claim_blocks_dispatch( + &run_and_prompting_target_context( + &tracker, + &config, + &workflow, + &state_store, + &issue.identifier, + IssueDispatchMode::Closeout, + ), + &issue.id, + &issue, + ) + .expect("active closeout claim guard should evaluate"), + "stopped active closeout lease should not block closeout dispatch" + ); + + let explicit_summary = orchestrator::run_target_issue_once(run_and_prompting_target_context( + &tracker, + &config, + &workflow, + &state_store, + &issue.identifier, + IssueDispatchMode::Closeout, + )) + .expect("explicit retained closeout identifier run should succeed") + .expect("explicit closeout should accept the stopped active closeout lease"); + + assert_eq!(explicit_summary.dispatch_mode, IssueDispatchMode::Closeout); + + let summary = orchestrator::run_target_issue_once_with_inferred_dispatch( + run_and_prompting_target_context( + &tracker, + &config, + &workflow, + &state_store, + &issue.identifier, + IssueDispatchMode::Normal, + ), + ) + .expect("targeted retained closeout identifier run should succeed") + .expect("stopped active closeout lease should not hide the closeout candidate"); + + assert_eq!(summary.issue_id, issue.id); + assert_eq!(summary.issue_identifier, issue.identifier); + assert_eq!(summary.dispatch_mode, IssueDispatchMode::Closeout); + assert_eq!(summary.run_id, "run-1"); + assert_eq!(summary.attempt_number, 1); +} + #[test] fn targeted_identifier_dispatch_rejects_different_status_visible_closeout_lane() { let (temp_dir, base_config, workflow) = temp_project_layout();