diff --git a/bot-components/utils/String_utils.ml b/bot-components/utils/String_utils.ml index 0ea0bc13..92a17a97 100644 --- a/bot-components/utils/String_utils.ml +++ b/bot-components/utils/String_utils.ml @@ -48,6 +48,113 @@ let first_line_of_string s = let remove_between s i j = String.sub ~pos:0 ~len:i s ^ String.sub s ~pos:j ~len:(String.length s - j) +type quote = Single | Double + +let split_shell_words ~preserve_syntax input = + let is_whitespace = function + | ' ' | '\t' | '\n' | '\r' | '\011' | '\012' -> + true + | _ -> + false + in + let buffer = Buffer.create (String.length input) in + let add_syntax char = if preserve_syntax then Buffer.add_char buffer char in + let finish_token token_started tokens = + if token_started then Buffer.contents buffer :: tokens else tokens + in + let length = String.length input in + let rec split index quote token_started tokens = + if index = length then + match quote with + | Some delimiter -> + Error + (Printf.sprintf "unterminated %c quote" + (match delimiter with Single -> '\'' | Double -> '"') ) + | None -> + Ok (List.rev (finish_token token_started tokens)) + else + let char = input.[index] in + match quote with + | None when is_whitespace char -> + let tokens = finish_token token_started tokens in + Buffer.clear buffer ; + split (index + 1) None false tokens + | None when Char.equal char '\\' -> + if index + 1 = length then Error "trailing escape character" + else + let escaped = input.[index + 1] in + if Char.equal escaped '\n' then + split (index + 2) None token_started tokens + else ( + add_syntax char ; + Buffer.add_char buffer escaped ; + split (index + 2) None true tokens ) + | None when Char.equal char '\'' -> + add_syntax char ; + split (index + 1) (Some Single) true tokens + | None when Char.equal char '"' -> + add_syntax char ; + split (index + 1) (Some Double) true tokens + | None -> + Buffer.add_char buffer char ; + split (index + 1) None true tokens + | Some Single when Char.equal char '\'' -> + add_syntax char ; + split (index + 1) None true tokens + | Some Single -> + Buffer.add_char buffer char ; + split (index + 1) quote true tokens + | Some Double when Char.equal char '"' -> + add_syntax char ; + split (index + 1) None true tokens + | Some Double when Char.equal char '\\' -> + if index + 1 = length then Error "unterminated \" quote" + else + let escaped = input.[index + 1] in + if Char.equal escaped '\n' then split (index + 2) quote true tokens + else if List.mem ['"'; '\\'; '$'; '`'] escaped ~equal:Char.equal + then ( + add_syntax char ; + Buffer.add_char buffer escaped ; + split (index + 2) quote true tokens ) + else ( + Buffer.add_char buffer char ; + split (index + 1) quote true tokens ) + | Some Double -> + Buffer.add_char buffer char ; + split (index + 1) quote true tokens + in + split 0 None false [] + +let split_on_unquoted_whitespace input = + split_shell_words ~preserve_syntax:true input + +let parse_key_value_arguments input = + match split_shell_words ~preserve_syntax:false input with + | Error _ as error -> + error + | Ok arguments -> + let rec parse parsed = function + | [] -> + Ok (List.rev parsed) + | argument :: arguments -> ( + match Stdlib.String.index_opt argument '=' with + | None when String.is_empty argument -> + Error (Printf.sprintf "argument %S has an empty key" argument) + | None -> + parse ((argument, None) :: parsed) arguments + | Some 0 -> + Error (Printf.sprintf "argument %S has an empty key" argument) + | Some separator -> + let key = String.sub argument ~pos:0 ~len:separator in + let value = + String.sub argument ~pos:(separator + 1) + ~len:(String.length argument - separator - 1) + in + parse ((key, Some value) :: parsed) arguments ) + in + parse [] arguments + (******************************************************************************) (* Formatting Functions *) (******************************************************************************) diff --git a/bot-components/utils/String_utils.mli b/bot-components/utils/String_utils.mli index 05e27c4e..48cac01d 100644 --- a/bot-components/utils/String_utils.mli +++ b/bot-components/utils/String_utils.mli @@ -23,6 +23,22 @@ val first_line_of_string : string -> string val remove_between : string -> int -> int -> string +val split_on_unquoted_whitespace : string -> (string list, string) Result.t +(** [split_on_unquoted_whitespace input] splits [input] at ASCII whitespace + outside single or double quotes. Shell quote and escape syntax is recognized + and preserved in the returned tokens. + + Returns an error for an unterminated quote or a trailing backslash. *) + +val parse_key_value_arguments : + string -> ((string * string option) list, string) Result.t +(** [parse_key_value_arguments input] parses shell-style [key=value] words. + Quote and escape syntax is consumed. A missing equal sign produces a [None] + value; an equal sign produces [Some value], including [Some ""] for an + explicitly empty value. Values may contain additional equal signs. + + Returns an error if quoting is malformed or an argument has an empty key. *) + (* ========================================================================== *) (* Formatting Functions *) (* ========================================================================== *) diff --git a/src/utils/bench.ml b/src/utils/bench.ml index 0170555f..b167f2ce 100644 --- a/src/utils/bench.ml +++ b/src/utils/bench.ml @@ -7,6 +7,26 @@ open HTTP_utils open String_utils open Lwt.Infix +type args = (string * string option) list + +let parse ~github_bot_name body = + if + string_match + ~regexp: + ( f "@%s:? [Bb]ench\\( *$\\| +\\(.*\\(\n.+\\)*\\)\\(\n\n\\|$\\)\\)" + @@ Str.quote github_bot_name ) + body + then + match Str.matched_group 2 body with + | exception _ -> + Some (Result.Ok []) + | args -> + Some + (Result.map_error (parse_key_value_arguments args) ~f:(fun error -> + f "bench command could not parse key-value arguments: %s" error ) + ) + else None + let parse_quantity table table_name = let regexp = {|.*TOP \([0-9]*\)|} in if string_match ~regexp table then diff --git a/src/utils/bench.mli b/src/utils/bench.mli index b53fe4b8..ab003801 100644 --- a/src/utils/bench.mli +++ b/src/utils/bench.mli @@ -1,5 +1,9 @@ open Base +type args = (string * string option) list + +val parse : github_bot_name:string -> string -> (args, string) Result.t option + module BenchResults : sig type t = { summary_table: string diff --git a/src/webhooks/github.ml b/src/webhooks/github.ml index 9fac3441..832dff45 100644 --- a/src/webhooks/github.ml +++ b/src/webhooks/github.ml @@ -65,6 +65,20 @@ let handle_push_event_for_repos ~bot_info ~key ~app_id ~install_id ~owner ~repo | _ -> Server.respond_string ~status:`OK ~body:"Ignoring push event." () +module Commands = struct + type bench_args = Bench.args + + type t = + | RunCI of {full_ci: bool option} + | Merge + | Bench of bench_args + | ResumeMinimize of (string * string list * Minimize_parser.minimize_parsed) + | Minimize of (string * string list) + | ParseError of string +end + +open Commands + (* Handles all comment-related events (minimization, CI commands, bench commands, etc.)*) let handle_comment_created ~bot_info ~key ~app_id ~github_bot_name ~gitlab_mapping ~github_mapping ~install_id @@ -91,25 +105,57 @@ let handle_comment_created ~bot_info ~key ~app_id ~github_bot_name |> Lwt.async ; Server.respond_string ~status:`OK ~body:"Handling minimization." () | None -> ( - (* Since both ci minimization resumption and ci - minimization will match the resumption string, and we - don't want to parse "resume" as an option, we test - resumption first *) - match resume_ci_minimize_text_of_body body with - | Some (options, requests, bug_file) -> - (fun () -> - init_git_bare_repository ~bot_info - >>= fun () -> - Bot_components.Github_installations.action_as_github_app ~bot_info - ~key ~app_id ~owner:comment_info.issue.issue.owner (fun ~bot_info -> - Minimization.ci_minimize ~bot_info ~comment_info ~requests - ~comment_on_error:true ~options ~bug_file:(Some bug_file) ) ) - |> Lwt.async ; - Server.respond_string ~status:`OK - ~body:"Handling CI minimization resumption." () - | None -> ( - match ci_minimize_text_of_body body with - | Some (options, requests) -> + let parse_run_ci body = + if + string_match + ~regexp: + ( f "@%s:? [Rr]un \\(full\\|light\\|\\) ?[Cc][Ii]" + @@ Str.quote github_bot_name ) + body + then + match Str.matched_group 1 body with + | "full" -> + Some (RunCI {full_ci= Some true}) + | "light" -> + Some (RunCI {full_ci= Some false}) + | "" -> + Some (RunCI {full_ci= None}) + | conf -> + Some + (ParseError + (f "run ci command: unknown CI configuration %S" conf) ) + else None + in + let parse_merge body = + if + string_match + ~regexp:(f "@%s:? [Mm]erge now" @@ Str.quote github_bot_name) + body + then Some Merge + else None + in + let parse () = + let open Option in + (* Since both ci minimization resumption and ci minimization will match the + resumption string, and we don't want to parse "resume" as an option, we + test resumption first *) + List.find_map + ~f:(fun fn -> fn body) + [ (fun body -> + resume_ci_minimize_text_of_body body >>| fun x -> ResumeMinimize x ) + ; (fun body -> ci_minimize_text_of_body body >>| fun x -> Minimize x) + ; parse_run_ci + ; parse_merge + ; (fun body -> + Bench.parse ~github_bot_name body + >>| function + | Result.Ok args -> + Commands.Bench args + | Result.Error error -> + ParseError error ) ] + in + match parse () with + | Some (ResumeMinimize (options, requests, bug_file)) -> (fun () -> init_git_bare_repository ~bot_info >>= fun () -> @@ -117,98 +163,78 @@ let handle_comment_created ~bot_info ~key ~app_id ~github_bot_name ~key ~app_id ~owner:comment_info.issue.issue.owner (fun ~bot_info -> Minimization.ci_minimize ~bot_info ~comment_info ~requests - ~comment_on_error:true ~options ~bug_file:None ) ) + ~comment_on_error:true ~options ~bug_file:(Some bug_file) ) ) |> Lwt.async ; - Server.respond_string ~status:`OK ~body:"Handling CI minimization." () - | None -> - if - string_match - ~regexp: - ( f "@%s:? [Rr]un \\(full\\|light\\|\\) ?[Cc][Ii]" - @@ Str.quote github_bot_name ) - body - && comment_info.issue.pull_request - && String.equal comment_info.issue.issue.owner "rocq-prover" - && String.equal comment_info.issue.issue.repo "rocq" - && Option.is_some install_id - then - let full_ci = - match Str.matched_group 1 body with - | "full" -> - Some true - | "light" -> - Some false - | "" -> - None - | _ -> - failwith "Impossible group value." - in + Server.respond_string ~status:`OK + ~body:"Handling CI minimization resumption." () + | Some (Minimize (options, requests)) -> + (fun () -> init_git_bare_repository ~bot_info >>= fun () -> Bot_components.Github_installations.action_as_github_app ~bot_info ~key ~app_id ~owner:comment_info.issue.issue.owner - (Pr_sync.run_ci_action ~comment_info ?full_ci ~gitlab_mapping - ~github_mapping () ) - else if - string_match - ~regexp:(f "@%s:? [Mm]erge now" @@ Str.quote github_bot_name) - body - && comment_info.issue.pull_request - && String.equal comment_info.issue.issue.owner "rocq-prover" - && String.equal comment_info.issue.issue.repo "rocq" - && Option.is_some install_id - then ( - (fun () -> - Bot_components.Github_installations.action_as_github_app ~bot_info - ~key ~app_id ~owner:comment_info.issue.issue.owner - (fun ~bot_info -> - GitHub_automation.merge_pull_request_action ~bot_info - comment_info ) ) - |> Lwt.async ; - Server.respond_string ~status:`OK - ~body:(f "Received a request to merge the PR.") - () ) - else if - string_match - ~regexp:(f "@%s:? [Bb]ench native" @@ Str.quote github_bot_name) - body - && comment_info.issue.pull_request - && String.equal comment_info.issue.issue.owner "rocq-prover" - && String.equal comment_info.issue.issue.repo "rocq" - && Option.is_some install_id - then ( - (fun () -> - Bot_components.Github_installations.action_as_github_app ~bot_info - ~key ~app_id ~owner:comment_info.issue.issue.owner - (fun ~bot_info -> - Bench.run_bench ~bot_info - ~key_value_pairs:[("coq_native", "yes")] - comment_info ) ) - |> Lwt.async ; - Server.respond_string ~status:`OK - ~body:(f "Received a request to start the bench.") - () ) - else if - string_match - ~regexp:(f "@%s:? [Bb]ench" @@ Str.quote github_bot_name) - body - && comment_info.issue.pull_request - && String.equal comment_info.issue.issue.owner "rocq-prover" - && String.equal comment_info.issue.issue.repo "rocq" - && Option.is_some install_id - then ( - (fun () -> - Bot_components.Github_installations.action_as_github_app ~bot_info - ~key ~app_id ~owner:comment_info.issue.issue.owner - (fun ~bot_info -> Bench.run_bench ~bot_info comment_info ) ) - |> Lwt.async ; - Server.respond_string ~status:`OK - ~body:(f "Received a request to start the bench.") - () ) - else - Server.respond_string ~status:`OK - ~body:(f "Unhandled comment: %s" body) - () ) ) + (fun ~bot_info -> + Minimization.ci_minimize ~bot_info ~comment_info ~requests + ~comment_on_error:true ~options ~bug_file:None ) ) + |> Lwt.async ; + Server.respond_string ~status:`OK ~body:"Handling CI minimization." () + | Some (RunCI {full_ci}) + when comment_info.issue.pull_request + && String.equal comment_info.issue.issue.owner "rocq-prover" + && String.equal comment_info.issue.issue.repo "rocq" + && Option.is_some install_id -> + init_git_bare_repository ~bot_info + >>= fun () -> + Bot_components.Github_installations.action_as_github_app ~bot_info + ~key ~app_id ~owner:comment_info.issue.issue.owner + (Pr_sync.run_ci_action ~comment_info ?full_ci ~gitlab_mapping + ~github_mapping () ) + | Some Merge + when comment_info.issue.pull_request + && String.equal comment_info.issue.issue.owner "rocq-prover" + && String.equal comment_info.issue.issue.repo "rocq" + && Option.is_some install_id -> + (fun () -> + Bot_components.Github_installations.action_as_github_app ~bot_info + ~key ~app_id ~owner:comment_info.issue.issue.owner + (fun ~bot_info -> + GitHub_automation.merge_pull_request_action ~bot_info + comment_info ) ) + |> Lwt.async ; + Server.respond_string ~status:`OK + ~body:(f "Received a request to merge the PR.") + () + | Some (Bench args) + when comment_info.issue.pull_request + && String.equal comment_info.issue.issue.owner "rocq-prover" + && String.equal comment_info.issue.issue.repo "rocq" + && Option.is_some install_id -> + let key_value_pairs = + List.map ~f:(fun (k, v) -> (k, Option.value ~default:"yes" v)) args + in + (fun () -> + Bot_components.Github_installations.action_as_github_app ~bot_info + ~key ~app_id ~owner:comment_info.issue.issue.owner + (fun ~bot_info -> + Bench.run_bench ~bot_info ~key_value_pairs comment_info ) ) + |> Lwt.async ; + Server.respond_string ~status:`OK + ~body:(f "Received a request to start the bench.") + () + | Some (ParseError error) -> + (fun () -> + GitHub_mutations.post_comment ~bot_info ~message:error + ~id:comment_info.issue.id + >>= Utils.report_on_posting_comment ) + |> Lwt.async ; + Server.respond_string ~status:`OK ~body:"Invalid bench arguments." () + | Some (RunCI _ | Merge | Bench _) -> + Server.respond_string ~status:`OK + ~body:"Command recognized but not allowed in this context." () + | None -> + Server.respond_string ~status:`OK + ~body:(f "Unhandled comment: %s" body) + () ) let handle_github_webhook ~bot_info ~key ~app_id ~github_bot_name ~gitlab_mapping ~github_mapping ~repo_config_table ~github_webhook_secret diff --git a/tests/dune b/tests/dune index 58458a8e..27a1da26 100644 --- a/tests/dune +++ b/tests/dune @@ -48,3 +48,8 @@ (name test_string_utils) (modules test_string_utils) (libraries alcotest bot-components base)) + +(test + (name test_bench) + (modules test_bench) + (libraries alcotest coq-bot.utils)) diff --git a/tests/test_bench.ml b/tests/test_bench.ml new file mode 100644 index 00000000..1bfdb1c1 --- /dev/null +++ b/tests/test_bench.ml @@ -0,0 +1,46 @@ +open Alcotest + +let args = list (pair string (option string)) + +let parse_result = option (result args string) + +let check_parse ~body ~expected () = + check parse_result body expected (Bench.parse ~github_bot_name:"coqbot" body) + +let () = + run "Bench parser tests" + [ ( "parse" + , [ ( "command without arguments" + , `Quick + , check_parse ~body:"@coqbot bench" ~expected:(Some (Ok [])) ) + ; ( "arguments through end of comment" + , `Quick + , check_parse + ~body:"@coqbot bench coq_native\ncoq_opam_packages=\"a b\"" + ~expected: + (Some + (Ok [("coq_native", None); ("coq_opam_packages", Some "a b")]) + ) ) + ; ( "multiline arguments until empty line" + , `Quick + , check_parse + ~body: + "@coqbot: Bench coq_native=yes\n\ + coq_opam_packages='a b'\n\n\ + This text is not part of the command." + ~expected: + (Some + (Ok + [ ("coq_native", Some "yes") + ; ("coq_opam_packages", Some "a b") ] ) ) ) + ; ( "malformed arguments" + , `Quick + , check_parse ~body:"@coqbot bench value=\"unterminated" + ~expected: + (Some + (Error + "bench command could not parse key-value arguments: \ + unterminated \" quote" ) ) ) + ; ( "another command" + , `Quick + , check_parse ~body:"@coqbot benchmark" ~expected:None ) ] ) ] diff --git a/tests/test_string_utils.ml b/tests/test_string_utils.ml index 7a7fb3a7..883da232 100644 --- a/tests/test_string_utils.ml +++ b/tests/test_string_utils.ml @@ -1,4 +1,6 @@ open Alcotest +open Base +open String_utils let test_strip_quoted_bot_name () = let input = @@ -12,7 +14,133 @@ let test_strip_quoted_bot_name () = in (check string) "strip_quoted_bot_name" expected got +let tokens = Alcotest.list Alcotest.string + +let key_values = + Alcotest.list + (Alcotest.pair Alcotest.string (Alcotest.option Alcotest.string)) + +let check_split ~input ~expected () = + match split_on_unquoted_whitespace input with + | Ok actual -> + Alcotest.check tokens input expected actual + | Error error -> + Alcotest.failf "expected %S to parse, but got: %s" input error + +let check_split_error ~input ~expected () = + match split_on_unquoted_whitespace input with + | Error actual -> + Alcotest.(check string) input expected actual + | Ok actual -> + Alcotest.failf "expected %S to fail, but got: [%s]" input + (String.concat ~sep:"; " actual) + +let check_arguments ~input ~expected () = + match parse_key_value_arguments input with + | Ok actual -> + Alcotest.check key_values input expected actual + | Error error -> + Alcotest.failf "expected %S to parse, but got: %s" input error + +let check_argument_error ~input ~expected () = + match parse_key_value_arguments input with + | Error actual -> + Alcotest.(check string) input expected actual + | Ok _ -> + Alcotest.failf "expected %S to fail" input + let () = - run "String_utils tests" + Alcotest.run "String_utils tests" [ ( "strip_quoted_bot_name" - , [test_case "quoted bot name" `Quick test_strip_quoted_bot_name] ) ] + , [test_case "quoted bot name" `Quick test_strip_quoted_bot_name] ) + ; ( "split_on_unquoted_whitespace" + , [ ("empty input", `Quick, check_split ~input:"" ~expected:[]) + ; ( "unquoted arguments" + , `Quick + , check_split ~input:"x=foo y=true" ~expected:["x=foo"; "y=true"] ) + ; ( "double-quoted whitespace" + , `Quick + , check_split ~input:{|x="foo bar" y=true|} + ~expected:[{|x="foo bar"|}; "y=true"] ) + ; ( "single-quoted whitespace" + , `Quick + , check_split ~input:"x='foo bar' y=true" + ~expected:["x='foo bar'"; "y=true"] ) + ; ( "repeated whitespace" + , `Quick + , check_split ~input:" x=foo\t\ty=true " + ~expected:["x=foo"; "y=true"] ) + ; ( "escaped quote" + , `Quick + , check_split ~input:{|x="foo \"bar\" baz" y=true|} + ~expected:[{|x="foo \"bar\" baz"|}; "y=true"] ) + ; ( "escaped whitespace" + , `Quick + , check_split ~input:{|x=foo\ bar y=true|} + ~expected:[{|x=foo\ bar|}; "y=true"] ) + ; ( "empty quoted arguments" + , `Quick + , check_split ~input:{|"" ''|} ~expected:[{|""|}; "''"] ) + ; ( "unterminated double quote" + , `Quick + , check_split_error ~input:{|x="foo bar|} + ~expected:"unterminated \" quote" ) + ; ( "trailing escape" + , `Quick + , check_split_error ~input:{|x=foo\|} + ~expected:"trailing escape character" ) ] ) + ; ( "parse_key_value_arguments" + , [ ( "quoted value" + , `Quick + , check_arguments ~input:{|x="foo bar" y=true|} + ~expected:[("x", Some "foo bar"); ("y", Some "true")] ) + ; ( "single-quoted value" + , `Quick + , check_arguments ~input:"x='foo bar' y=true" + ~expected:[("x", Some "foo bar"); ("y", Some "true")] ) + ; ( "concatenated quoted text" + , `Quick + , check_arguments ~input:{|x=foo" bar" y='true'|} + ~expected:[("x", Some "foo bar"); ("y", Some "true")] ) + ; ( "quoted whole argument" + , `Quick + , check_arguments ~input:{|"x=foo bar" y=true|} + ~expected:[("x", Some "foo bar"); ("y", Some "true")] ) + ; ( "escaped whitespace" + , `Quick + , check_arguments ~input:{|x=foo\ bar y=true|} + ~expected:[("x", Some "foo bar"); ("y", Some "true")] ) + ; ( "escaped quote" + , `Quick + , check_arguments ~input:{|x="foo \"bar\""|} + ~expected:[("x", Some {|foo "bar"|})] ) + ; ( "literal nested quotes" + , `Quick + , check_arguments ~input:{|x='"foo bar"'|} + ~expected:[("x", Some {|"foo bar"|})] ) + ; ( "explicit empty values" + , `Quick + , check_arguments ~input:{|x= y=""|} + ~expected:[("x", Some ""); ("y", Some "")] ) + ; ( "additional equal signs" + , `Quick + , check_arguments ~input:"x=foo=bar" ~expected:[("x", Some "foo=bar")] + ) + ; ( "missing value" + , `Quick + , check_arguments ~input:"x=foo missing" + ~expected:[("x", Some "foo"); ("missing", None)] ) + ; ( "empty key" + , `Quick + , check_argument_error ~input:"=value" + ~expected:"argument \"=value\" has an empty key" ) + ; ( "empty quoted key" + , `Quick + , check_argument_error ~input:{|""|} + ~expected:"argument \"\" has an empty key" ) + ; ( "coq_opam_packages" + , `Quick + , check_arguments ~input:{|coq_opam_packages="a b c" coq_native|} + ~expected: + [("coq_opam_packages", Some "a b c"); ("coq_native", None)] ) ] + ) ]