Address subset of review comments.

src/bin/main.ml

@@ -123,7 +123,7 @@ let edit ~proc ~net ~fs () file port =
   Cohttp_eio.Server.run socket server ~on_error:log_warning
 
 let md ~fs ~net ~domain_mgr ~proc () no_run store conf file port fetcher jobs
-    src_dir env_override input_override =
+    src_dir environment_override input_override =
   let import_map =
     List.map
       (fun (k, v) ->
@@ -220,8 +220,8 @@ let md ~fs ~net ~domain_mgr ~proc () no_run store conf file port fetcher jobs
           | Error (msg, cb) -> (cb, `Stop msg))
       | `Run ->
           let cb, _result_block, stop =
-            Shark.Md.process_run_block ~env_override ~fs ~build_cache ~pool
-              store ast obuilder (code_block, block)
+            Shark.Md.process_run_block ~environment_override ~fs ~build_cache
+              ~pool store ast obuilder (code_block, block)
           in
           (cb, stop)
   in
@@ -346,7 +346,7 @@ let secrets =
   @@ Arg.info ~doc:"Provide a secret under the form $(b,id:file)."
        ~docv:"SECRET" [ "secret" ]
 
-let env_override =
+let environment_override =
   Arg.value
   @@ Arg.(opt_all (pair ~sep:'=' string string)) []
   @@ Arg.info
@@ -384,7 +384,7 @@ let md ~fs ~net ~domain_mgr ~proc ~clock =
     Term.(
       const (md ~fs ~net ~domain_mgr ~proc ~clock)
       $ setup_log $ no_run $ store $ Obuilder.Native_sandbox.cmdliner
-      $ markdown_file $ port $ fetcher $ jobs $ src_dir $ env_override
+      $ markdown_file $ port $ fetcher $ jobs $ src_dir $ environment_override
       $ input_override)
 
 let editor ~proc ~net ~fs ~clock =

src/lib/ast/leaf.mli

@@ -1,4 +1,4 @@
-(** A Leaf is an atomic exection unit the in the pipeline graph. *)
+(** A Leaf is an atomic execution unit the in the pipeline graph. *)
 
 type style = Command | Map
 type t [@@deriving sexp]

src/lib/md.ml

@@ -152,8 +152,8 @@ let get_paths ~fs (Obuilder.Store_spec.Store ((module Store), store)) hash
       in
       List.map find_files_in_store outputs
 
-let process_run_block ?(env_override = []) ~fs ~build_cache ~pool store ast
-    (Builder ((module Builder), builder)) (_code_block, block) =
+let process_run_block ?(environment_override = []) ~fs ~build_cache ~pool store
+    ast (Builder ((module Builder), builder)) (_code_block, block) =
   let hyperblock =
     Ast.find_hyperblock_from_block ast block
     |> Option.get ~err:"No hyperblock for run block"
@@ -222,9 +222,9 @@ let process_run_block ?(env_override = []) ~fs ~build_cache ~pool store ast
         | Error (`Failed (id, msg)) -> Error (Some id, msg)
       in
 
-      let process_single_command acc leaf =
-        let _, prev_state = acc in
-        let inputs = Leaf.inputs leaf in
+      let process_single_command acc command_leaf =
+        let _, previous_state = acc in
+        let inputs = Leaf.inputs command_leaf in
         let input_and_hashes =
           List.map
             (fun i -> (i, List.assoc_opt (Datafile.id i) input_map))
@@ -250,7 +250,7 @@ let process_run_block ?(env_override = []) ~fs ~build_cache ~pool store ast
               | Some hash -> get_paths ~fs store hash true !ref_fd_list
               | None ->
                   let current_hash =
-                    Run_block.ExecutionState.build_hash prev_state
+                    Run_block.ExecutionState.build_hash previous_state
                   in
                   get_paths ~fs store current_hash false !ref_fd_list)
             hash_to_input_map
@@ -274,14 +274,15 @@ let process_run_block ?(env_override = []) ~fs ~build_cache ~pool store ast
             match s with
             | [] ->
                 Fmt.failwith "Failed to find source files for input %s of %s" i
-                  (Command.name (Leaf.command leaf))
+                  (Command.name (Leaf.command command_leaf))
             | _ -> ())
           l;
-        let inputs = Leaf.to_string_for_inputs leaf l in
+        let inputs = Leaf.to_string_for_inputs command_leaf l in
         let processed_blocks =
           Fiber.List.map
-            (Run_block.process_single_command_execution prev_state env_override
-               leaf l obuilder_command_runner)
+            (Run_block.process_single_command_execution ~previous_state
+               ~environment_override ~command_leaf ~file_subs_map:l
+               ~run_f:obuilder_command_runner)
             inputs
         in
         let results, _es = acc in
@@ -290,17 +291,17 @@ let process_run_block ?(env_override = []) ~fs ~build_cache ~pool store ast
           | hd :: _ -> hd
           | [] ->
               Fmt.failwith "There were no processed blocks for %s"
-                (Command.name (Leaf.command leaf))
+                (Command.name (Leaf.command command_leaf))
         in
         (processed_blocks :: results, es)
       in
 
       let ids_and_output_and_cmd, _es =
         List.fold_left process_single_command
           ( [],
-            Run_block.ExecutionState.init build
-              (Fpath.to_string (Ast.default_container_path ast))
-              [] )
+            Run_block.ExecutionState.init ~build_hash:build
+              ~workdir:(Fpath.to_string (Ast.default_container_path ast))
+              ~environment:[] )
           commands
       in
       let last = List.hd ids_and_output_and_cmd in

src/lib/md.mli

@@ -31,7 +31,7 @@ val process_build_block :
   Cmarkit.Block.Code_block.t * Block.t * [ `Stop of string | `Continue ]
 
 val process_run_block :
-  ?env_override:(string * string) list ->
+  ?environment_override:(string * string) list ->
   fs:_ Eio.Path.t ->
   build_cache:Build_cache.t ->
   pool:unit Eio.Pool.t ->

src/lib/execution/run_block.ml → src/lib/run_block.ml

@@ -23,21 +23,23 @@ module ExecutionState = struct
   }
   [@@deriving sexp]
 
-  let v result build_hash success workdir environment =
+  let v ~result ~build_hash ~success ~workdir ~environment =
     { result; build_hash; success; workdir; environment }
 
-  let init build_hash default_path default_env =
+  let init ~build_hash ~workdir ~environment =
     {
       result = CommandResult.v ~build_hash "";
       build_hash;
       success = true;
-      workdir = default_path;
-      environment = default_env;
+      workdir;
+      environment;
     }
 
   let change_dir e dst =
-    let res = CommandResult.v ~build_hash:e.build_hash (Fmt.str "cd %s" dst) in
-    { e with result = res; workdir = dst }
+    let result =
+      CommandResult.v ~build_hash:e.build_hash (Fmt.str "cd %s" dst)
+    in
+    { e with result; workdir = dst }
 
   let update_env e key value =
     let res =
@@ -47,6 +49,7 @@ module ExecutionState = struct
     let updated_env = (key, value) :: List.remove_assoc key e.environment in
     { e with result = res; environment = updated_env }
 
+  let command_fail e result = { e with result; success = false }
   let result e = e.result
   let build_hash e = e.build_hash
   let success e = e.success
@@ -55,9 +58,9 @@ module ExecutionState = struct
   let pp ppf t = Sexplib.Sexp.pp_hum ppf (sexp_of_t t)
 end
 
-let process_single_command_execution previous_state env_override leaf
-    file_subs_map run_f expanded_command_string =
-  let command = Leaf.command leaf in
+let process_single_command_execution ~previous_state ~environment_override
+    ~command_leaf ~file_subs_map ~run_f expanded_command_string =
+  let command = Leaf.command command_leaf in
   match Command.name command with
   | "cd" ->
       (* If a command block is a call to `cd` we treat this similarly to Docker's
@@ -77,7 +80,8 @@ let process_single_command_execution previous_state env_override leaf
             let path = Fpath.to_string (List.nth args 0) in
             match List.assoc_opt path file_subs_map with
             | None -> path
-            | Some pl -> ( match pl with [] -> path | _ -> List.nth pl 0))
+            | Some [] -> path
+            | Some pl -> List.nth pl 0)
       in
       ExecutionState.change_dir previous_state inspected_path
   | "export" ->
@@ -90,23 +94,24 @@ let process_single_command_execution previous_state env_override leaf
         | None -> Fmt.failwith "Malformed export command: %a" Command.pp command
       in
       let value =
-        match List.assoc_opt key env_override with
+        match List.assoc_opt key environment_override with
         | None -> default_value
         | Some v -> v
       in
       ExecutionState.update_env previous_state key value
   | _ -> (
       (* Otherwise we run a command using obuilder or such *)
       let buf = Buffer.create 128 in
-      let res = run_f previous_state leaf expanded_command_string buf in
+      let res = run_f previous_state command_leaf expanded_command_string buf in
       match res with
       | Ok id ->
           ExecutionState.v
-            (CommandResult.v ~build_hash:id ~output:(Buffer.contents buf)
-               expanded_command_string)
-            id true
-            (ExecutionState.workdir previous_state)
-            (ExecutionState.env previous_state)
+            ~result:
+              (CommandResult.v ~build_hash:id ~output:(Buffer.contents buf)
+                 expanded_command_string)
+            ~build_hash:id ~success:true
+            ~workdir:(ExecutionState.workdir previous_state)
+            ~environment:(ExecutionState.env previous_state)
       | Error (id_opt, msg) -> (
           match id_opt with
           | Some id ->
@@ -115,16 +120,12 @@ let process_single_command_execution previous_state env_override leaf
                   ~output:(msg ^ "\n" ^ Buffer.contents buf)
                   expanded_command_string
               in
-              ExecutionState.v cmd_result id false
-                (ExecutionState.workdir previous_state)
-                (ExecutionState.env previous_state)
+              ExecutionState.command_fail previous_state cmd_result
           | None ->
               let old_id = ExecutionState.build_hash previous_state in
               let cmd_result =
                 CommandResult.v ~build_hash:old_id
                   ~output:(msg ^ "\n" ^ Buffer.contents buf)
                   expanded_command_string
               in
-              ExecutionState.v cmd_result old_id false
-                (ExecutionState.workdir previous_state)
-                (ExecutionState.env previous_state)))
+              ExecutionState.command_fail previous_state cmd_result))

src/lib/execution/run_block.mli → src/lib/run_block.mli

@@ -12,9 +12,19 @@ module ExecutionState : sig
   type t [@@deriving sexp]
 
   val v :
-    CommandResult.t -> string -> bool -> string -> (string * string) list -> t
+    result:CommandResult.t ->
+    build_hash:string ->
+    success:bool ->
+    workdir:string ->
+    environment:(string * string) list ->
+    t
+
+  val init :
+    build_hash:string ->
+    workdir:string ->
+    environment:(string * string) list ->
+    t
 
-  val init : string -> string -> (string * string) list -> t
   val result : t -> CommandResult.t
   val build_hash : t -> string
   val success : t -> bool
@@ -24,21 +34,22 @@ module ExecutionState : sig
 end
 
 val process_single_command_execution :
-  ExecutionState.t ->
-  (string * string) list ->
-  Leaf.t ->
-  (string * string list) list ->
-  (ExecutionState.t ->
-  Leaf.t ->
-  string ->
-  Buffer.t ->
-  (string, string option * string) result) ->
+  previous_state:ExecutionState.t ->
+  environment_override:(string * string) list ->
+  command_leaf:Leaf.t ->
+  file_subs_map:(string * string list) list ->
+  run_f:
+    (ExecutionState.t ->
+    Leaf.t ->
+    string ->
+    Buffer.t ->
+    (string, string option * string) result) ->
   string ->
   ExecutionState.t
-(** [process_single_command_execution previous_state environment_override leaf file_map task_runner command_string] 
-  evaluates the [command_string] based on the state of the previous exection [previous_state]. Certain commands
+(** [process_single_command_execution previous_state environment_override command_leaf file_map task_runner command_string] 
+  evaluates the [command_string] based on the state of the previous execution [previous_state]. Certain commands
   will just update the state machine (e.g., cd and export), but commands that are side effect driven will
-  invoke [task_runner] to do the actual exection - most likely with Obuilder, but the point here is to keep 
+  invoke [task_runner] to do the actual execution - most likely with Obuilder, but the point here is to keep 
   that stuff external to allow for easier testing.
   
   Currently this has a complicated interface as it was before part of the main md run look - the aim is over