diff --git a/bin/main.ml b/bin/main.ml index 91b8b4b5..155e6862 100644 --- a/bin/main.ml +++ b/bin/main.ml @@ -15,6 +15,7 @@ type config = { poll_interval : float; repo_root : string; max_concurrency : int; + start_mode : Patch_controller.start_mode; headless : bool; user_config : User_config.t; } @@ -2207,6 +2208,7 @@ let runner_fiber ~runtime ~env ~config ~pick_backend ~project_name ~pr_registry Runtime.update_orchestrator_returning runtime (fun orch -> let orch, effects, messages = Patch_controller.plan_tick_messages orch ~project_name ~gameplan + ~start_mode:config.start_mode in let pre_fire_agents = Base.List.filter_map messages @@ -3763,84 +3765,68 @@ let with_snapshot_load ~project_name config gameplan = values. *) let resolve_config ~project ~gameplan_path ~github_token ~backend ~model ~main_branch ~poll_interval ~(repo_root : string option) ~max_concurrency - ~headless = + ~start_mode ~headless = let repo_root_for_fresh = Repo_root.normalize (Base.Option.value repo_root ~default:".") in + let merge_cli_stored cli stored_val = + let c = Base.String.strip cli in + if Base.String.is_empty c then stored_val else c + in + let load_existing_config ~project_name = + if Project_store.project_exists project_name then + match Project_store.load_config ~project_name with + | Ok stored -> Ok (Some stored) + | Error msg -> Error [ Printf.sprintf "Error loading config: %s" msg ] + else Ok None + in let resolve_branch ~repo_root mb_opt = match mb_opt with | Some b -> b | None -> Branch.of_string (infer_default_branch ~repo_root) in match (project, gameplan_path) with - | None, None -> - let repo_root = repo_root_for_fresh in - let token, owner, repo = - resolve_github_credentials ~github_token ~repo_root - in - let project_name = - if Base.String.is_empty owner || Base.String.is_empty repo then "adhoc" - else Printf.sprintf "%s-%s" owner repo - in - let gameplan = - Gameplan. - { - project_name; - problem_statement = ""; - solution_summary = ""; - final_state_spec = ""; - patches = []; - current_state_analysis = ""; - explicit_opinions = ""; - acceptance_criteria = []; - open_questions = []; - functional_changes = []; - } - in - let backend, model = resolve_backend_model ~backend ~model in - let main_branch = resolve_branch ~repo_root main_branch in - Project_store.save_config ~project_name ~github_token:token - ~github_owner:owner ~github_repo:repo ~backend ~model - ~main_branch:(Branch.to_string main_branch) - ~poll_interval ~repo_root ~max_concurrency; - let config = - { - project = Some project_name; - backend; - model; - github_token = token; - github_owner = owner; - github_repo = repo; - main_branch; - poll_interval; - repo_root; - max_concurrency; - headless; - user_config = User_config.load ~github_owner:owner ~github_repo:repo; - } - in - with_snapshot_load ~project_name config gameplan - | _, Some gp_path -> ( - match Gameplan_parser.parse_file gp_path with - | Error msg -> Error [ Printf.sprintf "Error parsing gameplan: %s" msg ] - | Ok parsed -> - let gameplan = parsed.Gameplan_parser.gameplan in - let project_name = - match project with - | Some p -> p - | None -> gameplan.Gameplan.project_name - in + | None, None -> ( + match start_mode with + | Some _ -> + Error + [ + "--start-mode can only be used with --gameplan or a stored \ + project; ad-hoc mode has no gameplan dependencies."; + ] + | None -> let repo_root = repo_root_for_fresh in let token, owner, repo = resolve_github_credentials ~github_token ~repo_root in + let project_name = + if Base.String.is_empty owner || Base.String.is_empty repo then + "adhoc" + else Printf.sprintf "%s-%s" owner repo + in + let gameplan = + Gameplan. + { + project_name; + problem_statement = ""; + solution_summary = ""; + final_state_spec = ""; + patches = []; + current_state_analysis = ""; + explicit_opinions = ""; + acceptance_criteria = []; + open_questions = []; + functional_changes = []; + } + in let backend, model = resolve_backend_model ~backend ~model in let main_branch = resolve_branch ~repo_root main_branch in + let start_mode = Patch_controller.Greedy in Project_store.save_config ~project_name ~github_token:token ~github_owner:owner ~github_repo:repo ~backend ~model ~main_branch:(Branch.to_string main_branch) - ~poll_interval ~repo_root ~max_concurrency; - Project_store.save_gameplan_source ~project_name ~source_path:gp_path; + ~poll_interval ~repo_root ~max_concurrency + ~start_mode:(Patch_controller.start_mode_to_string start_mode); let config = { project = Some project_name; @@ -3853,12 +3839,117 @@ let resolve_config ~project ~gameplan_path ~github_token ~backend ~model poll_interval; repo_root; max_concurrency; + start_mode; headless; user_config = User_config.load ~github_owner:owner ~github_repo:repo; } in with_snapshot_load ~project_name config gameplan) + | _, Some gp_path -> ( + match Gameplan_parser.parse_file gp_path with + | Error msg -> Error [ Printf.sprintf "Error parsing gameplan: %s" msg ] + | Ok parsed -> ( + let gameplan = parsed.Gameplan_parser.gameplan in + let project_name = + match project with + | Some p -> p + | None -> gameplan.Gameplan.project_name + in + match load_existing_config ~project_name with + | Error errs -> Error errs + | Ok stored_opt -> ( + let repo_root = + match (repo_root, stored_opt) with + | Some rr, _ -> Repo_root.normalize rr + | None, Some stored -> + Repo_root.normalize stored.Project_store.repo_root + | None, None -> repo_root_for_fresh + in + let backend_str, model_str = + match stored_opt with + | None -> (backend, model) + | Some stored -> + ( merge_cli_stored backend stored.Project_store.backend, + merge_cli_stored model stored.Project_store.model ) + in + let backend, model = + resolve_backend_model ~backend:backend_str ~model:model_str + in + let token_input = + match stored_opt with + | None -> github_token + | Some stored -> + merge_cli_stored github_token + stored.Project_store.github_token + in + let token, inferred_owner, inferred_repo = + resolve_github_credentials ~github_token:token_input ~repo_root + in + let owner = + match stored_opt with + | None -> inferred_owner + | Some stored -> + let s = + Base.String.strip stored.Project_store.github_owner + in + if Base.String.is_empty s then inferred_owner else s + in + let repo = + match stored_opt with + | None -> inferred_repo + | Some stored -> + let s = + Base.String.strip stored.Project_store.github_repo + in + if Base.String.is_empty s then inferred_repo else s + in + let main_branch = + match (main_branch, stored_opt) with + | Some b, _ -> b + | None, Some stored -> + Branch.of_string stored.Project_store.main_branch + | None, None -> resolve_branch ~repo_root None + in + let start_mode_result = + match (start_mode, stored_opt) with + | Some mode, _ -> Ok mode + | None, Some stored -> + Patch_controller.start_mode_of_string + stored.Project_store.start_mode + | None, None -> Ok Patch_controller.Greedy + in + match start_mode_result with + | Error msg -> + Error [ Printf.sprintf "Error loading config: %s" msg ] + | Ok start_mode -> + Project_store.save_config ~project_name ~github_token:token + ~github_owner:owner ~github_repo:repo ~backend ~model + ~main_branch:(Branch.to_string main_branch) + ~poll_interval ~repo_root ~max_concurrency + ~start_mode: + (Patch_controller.start_mode_to_string start_mode); + Project_store.save_gameplan_source ~project_name + ~source_path:gp_path; + let config = + { + project = Some project_name; + backend; + model; + github_token = token; + github_owner = owner; + github_repo = repo; + main_branch; + poll_interval; + repo_root; + max_concurrency; + start_mode; + headless; + user_config = + User_config.load ~github_owner:owner ~github_repo:repo; + } + in + with_snapshot_load ~project_name config gameplan))) | Some proj, None -> ( if not (Project_store.project_exists proj) then Error @@ -3881,13 +3972,9 @@ let resolve_config ~project ~gameplan_path ~github_token ~backend ~model match Project_store.load_config ~project_name:proj with | Error msg -> Error [ Printf.sprintf "Error loading config: %s" msg ] - | Ok stored -> + | Ok stored -> ( (* CLI flags override stored config; stored config overrides git-remote inference *) - let merge_cli_stored cli stored_val = - let c = Base.String.strip cli in - if Base.String.is_empty c then stored_val else c - in let resolved_backend_str = merge_cli_stored backend stored.Project_store.backend in @@ -3898,64 +3985,83 @@ let resolve_config ~project ~gameplan_path ~github_token ~backend ~model resolve_backend_model ~backend:resolved_backend_str ~model:resolved_model_str in - let token_from_stored = - merge_cli_stored github_token - stored.Project_store.github_token - in - (* Always route through [Repo_root.normalize] — including - the stored value — so legacy configs that persisted a - worktree path (or a trailing [/.]) self-heal on load. *) - let repo_root = - match repo_root with - | Some rr -> Repo_root.normalize rr - | None -> Repo_root.normalize stored.Project_store.repo_root - in - let token, inferred_owner, inferred_repo = - resolve_github_credentials ~github_token:token_from_stored - ~repo_root - in - let owner = - let s = - Base.String.strip stored.Project_store.github_owner - in - if Base.String.is_empty s then inferred_owner else s - in - let repo = - let s = - Base.String.strip stored.Project_store.github_repo - in - if Base.String.is_empty s then inferred_repo else s - in - let branch = - match main_branch with - | Some b -> b - | None -> Branch.of_string stored.Project_store.main_branch - in - (* Persist the resolved config so the next launch without - CLI overrides picks up the current values. *) - Project_store.save_config ~project_name:proj - ~github_token:token ~github_owner:owner ~github_repo:repo - ~backend ~model ~main_branch:(Branch.to_string branch) - ~poll_interval:stored.Project_store.poll_interval ~repo_root - ~max_concurrency:stored.Project_store.max_concurrency; - let config = - { - project = Some proj; - backend; - model; - github_token = token; - github_owner = owner; - github_repo = repo; - main_branch = branch; - poll_interval = stored.Project_store.poll_interval; - repo_root; - max_concurrency = stored.Project_store.max_concurrency; - headless; - user_config = - User_config.load ~github_owner:owner ~github_repo:repo; - } + let start_mode_result = + match start_mode with + | Some mode -> Ok mode + | None -> + Patch_controller.start_mode_of_string + stored.Project_store.start_mode in - with_snapshot_load ~project_name:proj config gameplan)) + match start_mode_result with + | Error msg -> + Error [ Printf.sprintf "Error loading config: %s" msg ] + | Ok start_mode -> + let token_from_stored = + merge_cli_stored github_token + stored.Project_store.github_token + in + (* Always route through [Repo_root.normalize] — including + the stored value — so legacy configs that persisted a + worktree path (or a trailing [/.]) self-heal on load. *) + let repo_root = + match repo_root with + | Some rr -> Repo_root.normalize rr + | None -> + Repo_root.normalize stored.Project_store.repo_root + in + let token, inferred_owner, inferred_repo = + resolve_github_credentials + ~github_token:token_from_stored ~repo_root + in + let owner = + let s = + Base.String.strip stored.Project_store.github_owner + in + if Base.String.is_empty s then inferred_owner else s + in + let repo = + let s = + Base.String.strip stored.Project_store.github_repo + in + if Base.String.is_empty s then inferred_repo else s + in + let branch = + match main_branch with + | Some b -> b + | None -> + Branch.of_string stored.Project_store.main_branch + in + (* Persist the resolved config so the next launch without + CLI overrides picks up the current values. *) + Project_store.save_config ~project_name:proj + ~github_token:token ~github_owner:owner + ~github_repo:repo ~backend ~model + ~main_branch:(Branch.to_string branch) + ~poll_interval:stored.Project_store.poll_interval + ~repo_root + ~max_concurrency:stored.Project_store.max_concurrency + ~start_mode: + (Patch_controller.start_mode_to_string start_mode); + let config = + { + project = Some proj; + backend; + model; + github_token = token; + github_owner = owner; + github_repo = repo; + main_branch = branch; + poll_interval = stored.Project_store.poll_interval; + repo_root; + max_concurrency = stored.Project_store.max_concurrency; + start_mode; + headless; + user_config = + User_config.load ~github_owner:owner + ~github_repo:repo; + } + in + with_snapshot_load ~project_name:proj config gameplan))) let run_with_config ~no_lock (config : config) gameplan existing_snapshot = let project_name = @@ -4307,10 +4413,11 @@ let run_with_config ~no_lock (config : config) gameplan existing_snapshot = let run ~project ~gameplan_path ~github_token ~backend ~model ~(main_branch : Branch.t option) ~poll_interval ~(repo_root : string option) - ~max_concurrency ~headless ~no_lock = + ~max_concurrency ~start_mode ~headless ~no_lock = match resolve_config ~project ~gameplan_path ~github_token ~backend ~model ~main_branch ~poll_interval ~repo_root ~max_concurrency ~headless + ~start_mode with | Error errs -> Base.List.iter errs ~f:(fun e -> Printf.eprintf "Error: %s\n" e); @@ -4400,6 +4507,22 @@ let max_concurrency_arg = ~doc:"Maximum number of concurrent backend sessions (default: 5)." ~env:(Cmd.Env.info "ONTON_MAX_CONCURRENCY")) +let start_mode_arg = + let open Cmdliner in + let mode = + Arg.enum + [ ("naive", Patch_controller.Naive); ("greedy", Patch_controller.Greedy) ] + in + Arg.( + value + & opt (some mode) None + & info [ "start-mode" ] ~docv:"MODE" + ~doc: + "Patch start scheduling mode: [greedy] may start downstream patches \ + on unmerged dependency branches; [naive] starts a patch only after \ + all dependencies have merged. This option is valid only with \ + --gameplan or a stored project.") + let headless_arg = let open Cmdliner in Arg.( @@ -4428,7 +4551,8 @@ let no_lock_arg = let main_cmd = let open Cmdliner in let run_cmd project gameplan_path github_token backend model main_branch - poll_interval repo_root max_concurrency headless upload_debug no_lock = + poll_interval repo_root max_concurrency start_mode headless upload_debug + no_lock = if upload_debug then ( match project with | None -> @@ -4453,13 +4577,14 @@ let main_cmd = run ~project ~gameplan_path ~github_token ~backend:(Base.String.strip backend) ~model:(Base.String.strip model) ~main_branch ~poll_interval ~repo_root - ~max_concurrency ~headless ~no_lock + ~max_concurrency ~start_mode ~headless ~no_lock in let term = Term.( const run_cmd $ project_arg $ gameplan_path_arg $ github_token_arg $ backend_arg $ model_arg $ main_branch_arg $ poll_interval_arg $ repo_arg - $ max_concurrency_arg $ headless_arg $ upload_debug_arg $ no_lock_arg) + $ max_concurrency_arg $ start_mode_arg $ headless_arg $ upload_debug_arg + $ no_lock_arg) in let info = Cmd.info "onton" ~version:Version.s diff --git a/lib/patch_controller.ml b/lib/patch_controller.ml index ca3f021d..57bceef4 100644 --- a/lib/patch_controller.ml +++ b/lib/patch_controller.ml @@ -57,6 +57,12 @@ type poll_observation = { worktree_path : string option; } +type start_mode = Patch_controller_core.start_mode = Naive | Greedy +[@@deriving show, eq, sexp_of] + +let start_mode_to_string = Patch_controller_core.start_mode_to_string +let start_mode_of_string = Patch_controller_core.start_mode_of_string + let discovery_intents orch = Orchestrator.all_agents orch |> List.filter_map ~f:(fun (agent : Patch_agent.t) -> @@ -295,7 +301,11 @@ let branch_map_of_patches patches = "Patch_controller.plan_actions: duplicate patch id %s" (Patch_id.to_string p.Patch.id))) -let plan_action_for_patch t ~branch_map patch_id = +let start_deps_satisfied t patch_id ~has_merged ~has_pr ~start_mode = + Patch_controller_core.start_deps_satisfied (Orchestrator.graph t) patch_id + ~has_merged ~has_pr ~start_mode + +let plan_action_for_patch t ~branch_map ~start_mode patch_id = let agent = Orchestrator.agent t patch_id in let has_merged pid = (Orchestrator.agent t pid).Patch_agent.merged in let has_pr pid = Patch_agent.has_pr (Orchestrator.agent t pid) in @@ -304,7 +314,7 @@ let plan_action_for_patch t ~branch_map patch_id = && (not agent.Patch_agent.busy) && (not agent.Patch_agent.merged) && (not (Patch_agent.needs_intervention agent)) - && Graph.deps_satisfied (Orchestrator.graph t) patch_id ~has_merged ~has_pr + && start_deps_satisfied t patch_id ~has_merged ~has_pr ~start_mode then let branch_of pid = match Map.find branch_map pid with @@ -380,7 +390,7 @@ let reconcile_action_message t action = let msg = Option.value_exn (Orchestrator.find_message t msg.message_id) in (t, msg) -let reconcile_messages t ~patches = +let reconcile_messages ?(start_mode = Greedy) t ~patches = let branch_map = branch_map_of_patches patches in let missing = Graph.all_patch_ids (Orchestrator.graph t) @@ -412,7 +422,9 @@ let reconcile_messages t ~patches = | Some msg when Orchestrator.equal_message_status msg.status Acked -> (acc, msg.message_id :: ids) | _ -> ( - match plan_action_for_patch acc ~branch_map patch_id with + match + plan_action_for_patch acc ~branch_map ~start_mode patch_id + with | None -> (acc, ids) | Some action -> let acc, msg = reconcile_action_message acc action in @@ -431,27 +443,32 @@ let reconcile_messages t ~patches = in (t, Orchestrator.runnable_messages t) -let plan_messages t ~patches = snd (reconcile_messages t ~patches) +let plan_messages ?(start_mode = Greedy) t ~patches = + snd (reconcile_messages ~start_mode t ~patches) -let plan_actions t ~patches = - plan_messages t ~patches +let plan_actions ?(start_mode = Greedy) t ~patches = + plan_messages ~start_mode t ~patches |> List.map ~f:(fun (msg : Orchestrator.patch_agent_message) -> msg.action) -let plan_tick_messages t ~project_name ~gameplan = +let plan_tick_messages ?(start_mode = Greedy) t ~project_name ~gameplan = let t, effects = reconcile_all t ~project_name ~gameplan in - let t, messages = reconcile_messages t ~patches:gameplan.Gameplan.patches in + let t, messages = + reconcile_messages ~start_mode t ~patches:gameplan.Gameplan.patches + in (t, effects, messages) -let plan_tick t ~project_name ~gameplan = - let t, effects, messages = plan_tick_messages t ~project_name ~gameplan in +let plan_tick ?(start_mode = Greedy) t ~project_name ~gameplan = + let t, effects, messages = + plan_tick_messages ~start_mode t ~project_name ~gameplan + in let actions = List.map messages ~f:(fun (msg : Orchestrator.patch_agent_message) -> msg.action) in (t, effects, actions) -let tick t ~project_name ~gameplan = - let t, effects, actions = plan_tick t ~project_name ~gameplan in +let tick ?(start_mode = Greedy) t ~project_name ~gameplan = + let t, effects, actions = plan_tick ~start_mode t ~project_name ~gameplan in let t = List.fold actions ~init:t ~f:(fun acc action -> Orchestrator.fire acc action) @@ -656,6 +673,90 @@ let make_orchestrator ~patch_id ~main_branch = let pid = Patch_id.of_string "p1" let main = Branch.of_string "main" +let scheduler_test_patch ?(dependencies = []) id branch = + { + Patch.id; + title = "test"; + description = "test"; + branch; + dependencies; + spec = ""; + acceptance_criteria = []; + changes = []; + files = []; + classification = ""; + test_stubs_introduced = []; + test_stubs_implemented = []; + complexity = None; + precedents = []; + } + +let has_start_action actions patch_id = + List.exists actions ~f:(function + | Orchestrator.Start (pid, _) -> Patch_id.equal pid patch_id + | Orchestrator.Respond _ | Orchestrator.Rebase _ -> false) + +let%test "plan_actions naive blocks patch with unmerged dependency PR" = + let dep_id = Patch_id.of_string "p1" in + let child_id = Patch_id.of_string "p2" in + let dep = scheduler_test_patch dep_id (Branch.of_string "dep-branch") in + let child = + scheduler_test_patch child_id + (Branch.of_string "child-branch") + ~dependencies:[ dep_id ] + in + let patches = [ dep; child ] in + let orch = Orchestrator.create ~patches ~main_branch:main in + let orch = Orchestrator.fire orch (Orchestrator.Start (dep_id, main)) in + let orch = Orchestrator.set_pr_number orch dep_id (Pr_number.of_int 41) in + let orch = Orchestrator.complete orch dep_id in + let actions = plan_actions ~start_mode:Naive orch ~patches in + not (has_start_action actions child_id) + +let%test "plan_actions greedy allows patch with sole unmerged dependency PR" = + let dep_id = Patch_id.of_string "p1" in + let child_id = Patch_id.of_string "p2" in + let dep = scheduler_test_patch dep_id (Branch.of_string "dep-branch") in + let child = + scheduler_test_patch child_id + (Branch.of_string "child-branch") + ~dependencies:[ dep_id ] + in + let patches = [ dep; child ] in + let orch = Orchestrator.create ~patches ~main_branch:main in + let orch = Orchestrator.fire orch (Orchestrator.Start (dep_id, main)) in + let orch = Orchestrator.set_pr_number orch dep_id (Pr_number.of_int 41) in + let orch = Orchestrator.complete orch dep_id in + let actions = plan_actions ~start_mode:Greedy orch ~patches in + has_start_action actions child_id + +let%test "plan_actions naive allows patch after dependencies merge" = + let dep_id = Patch_id.of_string "p1" in + let child_id = Patch_id.of_string "p2" in + let dep = scheduler_test_patch dep_id (Branch.of_string "dep-branch") in + let child = + scheduler_test_patch child_id + (Branch.of_string "child-branch") + ~dependencies:[ dep_id ] + in + let patches = [ dep; child ] in + let orch = Orchestrator.create ~patches ~main_branch:main in + let orch = Orchestrator.mark_merged orch dep_id in + let actions = plan_actions ~start_mode:Naive orch ~patches in + has_start_action actions child_id + +let%test "plan_actions naive keeps independent patches parallelizable" = + let p1 = + scheduler_test_patch (Patch_id.of_string "p1") (Branch.of_string "b1") + in + let p2 = + scheduler_test_patch (Patch_id.of_string "p2") (Branch.of_string "b2") + in + let patches = [ p1; p2 ] in + let orch = Orchestrator.create ~patches ~main_branch:main in + let actions = plan_actions ~start_mode:Naive orch ~patches in + has_start_action actions p1.Patch.id && has_start_action actions p2.Patch.id + let%test "reconcile_patch escalates repeated start discovery failures" = let patch, t = make_orchestrator ~patch_id:pid ~main_branch:main in let t = diff --git a/lib/patch_controller.mli b/lib/patch_controller.mli index b21e0b0d..60d84522 100644 --- a/lib/patch_controller.mli +++ b/lib/patch_controller.mli @@ -23,6 +23,15 @@ type poll_observation = { worktree_path : string option; } +type start_mode = Patch_controller_core.start_mode = Naive | Greedy +[@@deriving show, eq, sexp_of] + +val start_mode_to_string : start_mode -> string +(** Stable CLI/config spelling for start scheduling modes. *) + +val start_mode_of_string : string -> (start_mode, string) result +(** Parse a CLI/config spelling for start scheduling modes. *) + val discovery_intents : Orchestrator.t -> (Patch_id.t * Branch.t) list (** Patches that have run at least once ([has_session]) but lack a PR and are not merged. Returns [(patch_id, branch)] pairs for tick-based PR discovery @@ -65,11 +74,15 @@ val reconcile_all : (** Reconcile all gameplan patches. Ad-hoc patches are ignored. *) val plan_actions : - Orchestrator.t -> patches:Patch.t list -> Orchestrator.action list + ?start_mode:start_mode -> + Orchestrator.t -> + patches:Patch.t list -> + Orchestrator.action list (** Compute runnable actions from the current snapshot after reconciliation. This is the evergreen scheduler used by the main loop. *) val plan_messages : + ?start_mode:start_mode -> Orchestrator.t -> patches:Patch.t list -> Orchestrator.patch_agent_message list @@ -78,6 +91,7 @@ val plan_messages : actions become pending messages in the outbox. *) val plan_tick_messages : + ?start_mode:start_mode -> Orchestrator.t -> project_name:string -> gameplan:Gameplan.t -> @@ -86,6 +100,7 @@ val plan_tick_messages : runnable patch-agent messages for the same snapshot. *) val plan_tick : + ?start_mode:start_mode -> Orchestrator.t -> project_name:string -> gameplan:Gameplan.t -> @@ -94,6 +109,7 @@ val plan_tick : actions for the same snapshot. *) val tick : + ?start_mode:start_mode -> Orchestrator.t -> project_name:string -> gameplan:Gameplan.t -> diff --git a/lib/project_store.ml b/lib/project_store.ml index cba4ff55..4c4ef2ee 100644 --- a/lib/project_store.ml +++ b/lib/project_store.ml @@ -76,11 +76,12 @@ type stored_config = { poll_interval : float; repo_root : string; max_concurrency : int; + start_mode : string; } [@@deriving yojson] let save_config ~project_name ~github_token ~github_owner ~github_repo ~backend - ~model ~main_branch ~poll_interval ~repo_root ~max_concurrency = + ~model ~main_branch ~poll_interval ~repo_root ~max_concurrency ~start_mode = let dir = project_dir project_name in ensure_dir dir; let config = @@ -95,6 +96,7 @@ let save_config ~project_name ~github_token ~github_owner ~github_repo ~backend poll_interval; repo_root; max_concurrency; + start_mode; } in let json = yojson_of_stored_config config in @@ -134,6 +136,12 @@ let migrate_backend_model fields = in ("backend", `String backend) :: ("model", `String model) :: without +let migrate_start_mode fields = + let assoc = List.Assoc.find fields ~equal:String.equal in + match assoc "start_mode" with + | Some _ -> fields + | None -> ("start_mode", `String "greedy") :: fields + let load_config ~project_name = let path = config_path project_name in try @@ -146,7 +154,9 @@ let load_config ~project_name = let json = Yojson.Safe.from_string content in match json with | `Assoc fields -> - Ok (stored_config_of_yojson (`Assoc (migrate_backend_model fields))) + Ok + (stored_config_of_yojson + (`Assoc (fields |> migrate_backend_model |> migrate_start_mode))) | _ -> Ok (stored_config_of_yojson json) with exn -> Error (Stdlib.Printexc.to_string exn) diff --git a/lib/project_store.mli b/lib/project_store.mli index b0521f08..2646cdd3 100644 --- a/lib/project_store.mli +++ b/lib/project_store.mli @@ -33,6 +33,7 @@ type stored_config = { poll_interval : float; repo_root : string; max_concurrency : int; + start_mode : string; } [@@deriving yojson] @@ -47,6 +48,7 @@ val save_config : poll_interval:float -> repo_root:string -> max_concurrency:int -> + start_mode:string -> unit (** Persist project config to the data directory. Creates the directory if needed. *) diff --git a/lib_core/patch_controller_core.ml b/lib_core/patch_controller_core.ml new file mode 100644 index 00000000..da3ba567 --- /dev/null +++ b/lib_core/patch_controller_core.ml @@ -0,0 +1,20 @@ +open Base + +type start_mode = Naive | Greedy [@@deriving show, eq, sexp_of] + +let start_mode_to_string = function Naive -> "naive" | Greedy -> "greedy" + +let start_mode_of_string s = + match String.lowercase (String.strip s) with + | "naive" -> Ok Naive + | "greedy" -> Ok Greedy + | other -> + Error + (Printf.sprintf + "invalid start mode %S; expected \"naive\" or \"greedy\"" other) + +let start_deps_satisfied graph patch_id ~has_merged ~has_pr ~start_mode = + match start_mode with + | Greedy -> Graph.deps_satisfied graph patch_id ~has_merged ~has_pr + | Naive -> + Graph.deps graph patch_id |> List.for_all ~f:(fun dep -> has_merged dep) diff --git a/lib_core/patch_controller_core.mli b/lib_core/patch_controller_core.mli new file mode 100644 index 00000000..2e397f06 --- /dev/null +++ b/lib_core/patch_controller_core.mli @@ -0,0 +1,23 @@ +open Base +open Types + +type start_mode = Naive | Greedy [@@deriving show, eq, sexp_of] + +val start_mode_to_string : start_mode -> string +(** Stable CLI/config spelling for start scheduling modes. *) + +val start_mode_of_string : string -> (start_mode, string) Result.t +(** Parse a CLI/config spelling for start scheduling modes. *) + +val start_deps_satisfied : + Graph.t -> + Patch_id.t -> + has_merged:(Patch_id.t -> bool) -> + has_pr:(Patch_id.t -> bool) -> + start_mode:start_mode -> + bool +(** Whether [patch_id]'s dependencies allow starting under [start_mode]. + + [Greedy] allows a patch when every dependency has either merged or has an + open PR, and at most one dependency remains unmerged. [Naive] requires every + direct dependency to have merged. *) diff --git a/test/dune b/test/dune index 4ab0b30d..6a07a072 100644 --- a/test/dune +++ b/test/dune @@ -189,6 +189,13 @@ (ocamlopt_flags (:standard -w -40-42)) (ocamlc_flags (:standard -w -40-42))) +(test + (name test_patch_controller_properties) + (modules test_patch_controller_properties) + (libraries onton_core qcheck-core qcheck-core.runner) + (ocamlopt_flags (:standard -w -40-42)) + (ocamlc_flags (:standard -w -40-42))) + (test (name test_patch_controller_state_machine) (modules test_patch_controller_state_machine) diff --git a/test/test_patch_controller_properties.ml b/test/test_patch_controller_properties.ml new file mode 100644 index 00000000..db274813 --- /dev/null +++ b/test/test_patch_controller_properties.ml @@ -0,0 +1,185 @@ +open Base +open Onton_core +open Onton_core.Types +module Scheduler = Patch_controller_core + +let modes = [ Scheduler.Naive; Scheduler.Greedy ] +let patch_id i = Patch_id.of_string (Printf.sprintf "p%d" i) +let branch i = Branch.of_string (Printf.sprintf "branch-%d" i) + +let patch ?(dependencies = []) i = + { + Patch.id = patch_id i; + title = "test"; + description = "test"; + branch = branch i; + dependencies; + spec = ""; + acceptance_criteria = []; + changes = []; + files = []; + classification = ""; + test_stubs_introduced = []; + test_stubs_implemented = []; + complexity = None; + precedents = []; + } + +let chain_graph = + let dep = patch 1 in + let mid = patch ~dependencies:[ dep.Patch.id ] 2 in + let child = patch ~dependencies:[ mid.Patch.id ] 3 in + Graph.of_patches [ dep; mid; child ] + +let chain_dep = patch_id 1 +let chain_mid = patch_id 2 +let chain_child = patch_id 3 +let set_of_ids ids = Set.of_list (module Patch_id) ids +let in_set set id = Set.mem set id + +let deps_satisfied graph patch_id ~merged ~prs ~start_mode = + Scheduler.start_deps_satisfied graph patch_id + ~has_merged:(in_set (set_of_ids merged)) + ~has_pr:(in_set (set_of_ids prs)) + ~start_mode + +let can_start graph patch_id ~merged ~prs ~start_mode = + (not (List.mem merged patch_id ~equal:Patch_id.equal)) + && (not (List.mem prs patch_id ~equal:Patch_id.equal)) + && deps_satisfied graph patch_id ~merged ~prs ~start_mode + +let graph_of_patches_total patches = + try Graph.of_patches patches with Invalid_argument _ -> Graph.of_patches [] + +let prop_start_mode_parse_total = + QCheck2.Test.make ~name:"start_mode_of_string is total" ~count:1_000 + QCheck2.Gen.(string_size ~gen:printable (int_range 0 80)) + (fun s -> + ignore + (Scheduler.start_mode_of_string s + : (Scheduler.start_mode, string) Result.t); + true) + +let prop_start_mode_round_trip = + QCheck2.Test.make ~name:"start_mode string round-trips" + (QCheck2.Gen.oneof_list modes) (fun mode -> + match + Scheduler.start_mode_of_string (Scheduler.start_mode_to_string mode) + with + | Ok parsed -> Scheduler.equal_start_mode parsed mode + | Error _ -> false) + +let prop_chain_naive_blocks_mid_until_dep_merged = + QCheck2.Test.make + ~name:"chain: naive blocks mid until direct dependency merges" + QCheck2.Gen.(pair bool bool) + (fun (dep_has_pr, dep_merged) -> + let prs = if dep_has_pr then [ chain_dep ] else [] in + let merged = if dep_merged then [ chain_dep ] else [] in + Bool.equal + (deps_satisfied chain_graph chain_mid ~merged ~prs + ~start_mode:Scheduler.Naive) + dep_merged) + +let prop_chain_greedy_allows_mid_when_dep_has_pr_or_merged = + QCheck2.Test.make + ~name:"chain: greedy allows mid when sole dependency has PR or merged" + QCheck2.Gen.(pair bool bool) + (fun (dep_has_pr, dep_merged) -> + let prs = if dep_has_pr then [ chain_dep ] else [] in + let merged = if dep_merged then [ chain_dep ] else [] in + Bool.equal + (deps_satisfied chain_graph chain_mid ~merged ~prs + ~start_mode:Scheduler.Greedy) + (dep_has_pr || dep_merged)) + +let prop_chain_naive_blocks_child_until_mid_merged = + QCheck2.Test.make ~name:"chain: naive blocks child until each level is merged" + QCheck2.Gen.(pair bool bool) + (fun (mid_has_pr, mid_merged) -> + let prs = if mid_has_pr then [ chain_mid ] else [] in + let merged = chain_dep :: (if mid_merged then [ chain_mid ] else []) in + Bool.equal + (deps_satisfied chain_graph chain_child ~merged ~prs + ~start_mode:Scheduler.Naive) + mid_merged) + +let prop_chain_greedy_allows_child_when_mid_has_pr = + QCheck2.Test.make + ~name:"chain: greedy allows child when sole dependency has PR" + QCheck2.Gen.(pair bool bool) + (fun (mid_has_pr, mid_merged) -> + let prs = if mid_has_pr then [ chain_mid ] else [] in + let merged = chain_dep :: (if mid_merged then [ chain_mid ] else []) in + Bool.equal + (deps_satisfied chain_graph chain_child ~merged ~prs + ~start_mode:Scheduler.Greedy) + (mid_has_pr || mid_merged)) + +type generated_graph = { + graph : Graph.t; + ids : Patch_id.t list; + merged : Patch_id.t list; + prs : Patch_id.t list; +} + +let gen_generated_graph = + let open QCheck2.Gen in + let* n = int_range 1 7 in + let indices = List.init n ~f:(fun i -> i + 1) in + let rec gen_deps i = + if i <= n then + let* flags = list_size (return (i - 1)) bool in + let deps = + List.filter_mapi flags ~f:(fun j selected -> + if selected then Some (patch_id (j + 1)) else None) + in + let* rest = gen_deps (i + 1) in + return ((i, deps) :: rest) + else return [] + in + let* dep_specs = gen_deps 1 in + let patches = + List.map dep_specs ~f:(fun (i, dependencies) -> patch ~dependencies i) + in + let ids = List.map indices ~f:patch_id in + let* merged_flags = list_size (return n) bool in + let* pr_flags = list_size (return n) bool in + let merged = + List.filter_mapi merged_flags ~f:(fun i selected -> + if selected then Some (patch_id (i + 1)) else None) + in + let prs = + List.filter_mapi pr_flags ~f:(fun i selected -> + if selected then Some (patch_id (i + 1)) else None) + in + return { graph = graph_of_patches_total patches; ids; merged; prs } + +let prop_naive_startable_subset_of_greedy = + QCheck2.Test.make ~name:"naive-startable set is subset of greedy-startable" + ~count:500 gen_generated_graph (fun { graph; ids; merged; prs } -> + List.for_all ids ~f:(fun id -> + (not (can_start graph id ~merged ~prs ~start_mode:Scheduler.Naive)) + || can_start graph id ~merged ~prs ~start_mode:Scheduler.Greedy)) + +let prop_greedy_is_strictly_stronger_witness = + QCheck2.Test.make ~name:"greedy has a strict startability witness" + QCheck2.Gen.unit (fun () -> + deps_satisfied chain_graph chain_mid ~merged:[] ~prs:[ chain_dep ] + ~start_mode:Scheduler.Greedy + && not + (deps_satisfied chain_graph chain_mid ~merged:[] ~prs:[ chain_dep ] + ~start_mode:Scheduler.Naive)) + +let () = + [ + prop_start_mode_parse_total; + prop_start_mode_round_trip; + prop_chain_naive_blocks_mid_until_dep_merged; + prop_chain_greedy_allows_mid_when_dep_has_pr_or_merged; + prop_chain_naive_blocks_child_until_mid_merged; + prop_chain_greedy_allows_child_when_mid_has_pr; + prop_naive_startable_subset_of_greedy; + prop_greedy_is_strictly_stronger_witness; + ] + |> List.iter ~f:(fun test -> QCheck2.Test.check_exn test)