Small code improvements based on lint warnings

- add more noexcept requirements and enforce existing
- fixing inconsistencies related to function arguments
- remove redundant static keywords
- silencing excessive lint reporting in test cases

While there, make more getters const ref.

9 files changed, 70 insertions, 43 deletions

diff --git a/src/buildtool/common/action.hpp b/src/buildtool/common/action.hpp
index 350a1d2b..c134ae25 100644
--- a/src/buildtool/common/action.hpp
+++ b/src/buildtool/common/action.hpp
@@ -56,7 +56,9 @@ class Action {
                  1.0,
                  std::map<std::string, std::string>{}) {}
 
-    [[nodiscard]] auto Id() const noexcept -> ActionIdentifier { return id_; }
+    [[nodiscard]] auto Id() const noexcept -> ActionIdentifier const& {
+        return id_;
+    }
 
     [[nodiscard]] auto Command() && noexcept -> std::vector<std::string> {
         return std::move(command_);
@@ -67,10 +69,12 @@ class Action {
         return command_;
     }
 
-    [[nodiscard]] auto Cwd() const -> std::string { return cwd_; }
+    [[nodiscard]] auto Cwd() const noexcept -> std::string const& {
+        return cwd_;
+    }
 
     [[nodiscard]] auto Env() const& noexcept
-        -> std::map<std::string, std::string> {
+        -> std::map<std::string, std::string> const& {
         return env_;
     }
 
@@ -78,15 +82,20 @@ class Action {
         return std::move(env_);
     }
 
-    [[nodiscard]] auto IsTreeAction() const -> bool { return is_tree_; }
-    [[nodiscard]] auto MayFail() const -> std::optional<std::string> {
+    [[nodiscard]] auto IsTreeAction() const noexcept -> bool {
+        return is_tree_;
+    }
+    [[nodiscard]] auto MayFail() const noexcept
+        -> std::optional<std::string> const& {
         return may_fail_;
     }
-    [[nodiscard]] auto NoCache() const -> bool { return no_cache_; }
-    [[nodiscard]] auto TimeoutScale() const -> double { return timeout_scale_; }
+    [[nodiscard]] auto NoCache() const noexcept -> bool { return no_cache_; }
+    [[nodiscard]] auto TimeoutScale() const noexcept -> double {
+        return timeout_scale_;
+    }
 
     [[nodiscard]] auto ExecutionProperties() const& noexcept
-        -> std::map<std::string, std::string> {
+        -> std::map<std::string, std::string> const& {
         return execution_properties_;
     }
 
@@ -95,8 +104,8 @@ class Action {
         return std::move(execution_properties_);
     }
 
-    [[nodiscard]] static auto CreateTreeAction(ActionIdentifier const& id)
-        -> Action {
+    [[nodiscard]] static auto CreateTreeAction(
+        ActionIdentifier const& id) noexcept -> Action {
         return Action{id};
     }
 
@@ -111,7 +120,8 @@ class Action {
     double timeout_scale_{};
     std::map<std::string, std::string> execution_properties_{};
 
-    explicit Action(ActionIdentifier id) : id_{std::move(id)}, is_tree_{true} {}
+    explicit Action(ActionIdentifier id) noexcept
+        : id_{std::move(id)}, is_tree_{true} {}
 };
 
 #endif  // INCLUDED_SRC_BUILDTOOL_COMMON_ACTION_HPP
diff --git a/src/buildtool/execution_api/remote/bazel/bazel_api.cpp b/src/buildtool/execution_api/remote/bazel/bazel_api.cpp
index 07c5c3ef..e42c32e7 100644
--- a/src/buildtool/execution_api/remote/bazel/bazel_api.cpp
+++ b/src/buildtool/execution_api/remote/bazel/bazel_api.cpp
@@ -386,11 +386,19 @@ auto BazelApi::CreateAction(
     gsl::not_null<std::unordered_set<Artifact::ObjectInfo>*> done)
     const noexcept -> bool {
     std::unordered_set<Artifact::ObjectInfo> artifacts_info;
-    artifacts_info.reserve(all_artifacts_info.size());
-    for (auto const& info : all_artifacts_info) {
-        if (not done->contains(info)) {
-            artifacts_info.insert(info);
+    try {
+        artifacts_info.reserve(all_artifacts_info.size());
+        for (auto const& info : all_artifacts_info) {
+            if (not done->contains(info)) {
+                artifacts_info.emplace(info);
+            }
         }
+    } catch (std::exception const& ex) {
+        Logger::Log(
+            LogLevel::Error,
+            "BazelApi: Collecting the set of artifacts failed with:\n{}",
+            ex.what());
+        return false;
     }
     if (artifacts_info.empty()) {
         return true;  // Nothing to do
diff --git a/src/buildtool/execution_engine/dag/dag.cpp b/src/buildtool/execution_engine/dag/dag.cpp
index 17f4b3af..cb82ec55 100644
--- a/src/buildtool/execution_engine/dag/dag.cpp
+++ b/src/buildtool/execution_engine/dag/dag.cpp
@@ -148,7 +148,7 @@ auto DependencyGraph::AddAction(ActionDescription const& description) -> bool {
 
 auto DependencyGraph::AddAction(Action const& a) noexcept
     -> DependencyGraph::ActionNodeIdentifier {
-    auto id = a.Id();
+    auto const& id = a.Id();
     auto const action_it = action_ids_.find(id);
     if (action_it != action_ids_.end()) {
         return action_it->second;
diff --git a/src/buildtool/execution_engine/dag/dag.hpp b/src/buildtool/execution_engine/dag/dag.hpp
index 99a35f4b..7e2194ff 100644
--- a/src/buildtool/execution_engine/dag/dag.hpp
+++ b/src/buildtool/execution_engine/dag/dag.hpp
@@ -314,57 +314,60 @@ class DependencyGraph : DirectedAcyclicGraph {
             return true;
         }
 
-        [[nodiscard]] auto OutputFiles()
-            const& -> std::vector<NamedOtherNodePtr> const& {
+        [[nodiscard]] auto OutputFiles() const& noexcept
+            -> std::vector<NamedOtherNodePtr> const& {
             return output_files_;
         }
 
-        [[nodiscard]] auto OutputDirs()
-            const& -> std::vector<NamedOtherNodePtr> const& {
+        [[nodiscard]] auto OutputDirs() const& noexcept
+            -> std::vector<NamedOtherNodePtr> const& {
             return output_dirs_;
         }
 
-        [[nodiscard]] auto Dependencies()
-            const& -> std::vector<NamedOtherNodePtr> const& {
+        [[nodiscard]] auto Dependencies() const& noexcept
+            -> std::vector<NamedOtherNodePtr> const& {
             return dependencies_;
         }
 
-        [[nodiscard]] auto Command() const -> std::vector<std::string> {
+        [[nodiscard]] auto Command() const noexcept
+            -> std::vector<std::string> const& {
             return Content().Command();
         }
 
-        [[nodiscard]] auto Env() const -> std::map<std::string, std::string> {
+        [[nodiscard]] auto Env() const noexcept
+            -> std::map<std::string, std::string> const& {
             return Content().Env();
         }
 
-        [[nodiscard]] auto MayFail() const -> std::optional<std::string> {
+        [[nodiscard]] auto MayFail() const noexcept
+            -> std::optional<std::string> const& {
             return Content().MayFail();
         }
 
-        [[nodiscard]] auto TimeoutScale() const -> double {
+        [[nodiscard]] auto TimeoutScale() const noexcept -> double {
             return Content().TimeoutScale();
         }
 
-        [[nodiscard]] auto ExecutionProperties() const
-            -> std::map<std::string, std::string> {
+        [[nodiscard]] auto ExecutionProperties() const noexcept
+            -> std::map<std::string, std::string> const& {
             return Content().ExecutionProperties();
         }
 
-        [[nodiscard]] auto NoCache() const -> bool {
+        [[nodiscard]] auto NoCache() const noexcept -> bool {
             return Content().NoCache();
         }
 
-        [[nodiscard]] auto OutputFilePaths() const
+        [[nodiscard]] auto OutputFilePaths() const noexcept
             -> std::vector<Action::LocalPath> {
             return NodePaths(output_files_);
         }
 
-        [[nodiscard]] auto OutputDirPaths() const
+        [[nodiscard]] auto OutputDirPaths() const noexcept
             -> std::vector<Action::LocalPath> {
             return NodePaths(output_dirs_);
         }
 
-        [[nodiscard]] auto DependencyPaths() const
+        [[nodiscard]] auto DependencyPaths() const noexcept
             -> std::vector<Action::LocalPath> {
             return NodePaths(dependencies_);
         }
diff --git a/src/buildtool/execution_engine/executor/executor.hpp b/src/buildtool/execution_engine/executor/executor.hpp
index d0bc7aee..097284e1 100644
--- a/src/buildtool/execution_engine/executor/executor.hpp
+++ b/src/buildtool/execution_engine/executor/executor.hpp
@@ -972,11 +972,17 @@ class Rebuilder {
                 return artifacts_cached.error();
             }
             std::ostringstream msg{};
-            for (auto const& [path, info] : *artifacts.value()) {
-                auto const& info_cached = artifacts_cached.value()->at(path);
-                if (info != info_cached) {
-                    RecordFlakyAction(&msg, action, path, info, info_cached);
+            try {
+                for (auto const& [path, info] : *artifacts.value()) {
+                    auto const& info_cached =
+                        artifacts_cached.value()->at(path);
+                    if (info != info_cached) {
+                        RecordFlakyAction(
+                            &msg, action, path, info, info_cached);
+                    }
                 }
+            } catch (std::exception const& ex) {
+                return ex.what();
             }
             if (msg.tellp() > 0) {
                 stats.IncrementActionsFlakyCounter();
diff --git a/src/buildtool/serve_api/serve_service/source_tree.cpp b/src/buildtool/serve_api/serve_service/source_tree.cpp
index 567d61bb..5aae6e07 100644
--- a/src/buildtool/serve_api/serve_service/source_tree.cpp
+++ b/src/buildtool/serve_api/serve_service/source_tree.cpp
@@ -461,7 +461,7 @@ auto SourceTreeService::ResolveContentTree(
 }
 
 auto SourceTreeService::CommonImportToGit(
-    std::filesystem::path const& content_path,
+    std::filesystem::path const& root_path,
     std::string const& commit_message) -> expected<std::string, std::string> {
     // the repository path that imports the content must be separate from the
     // content path, to avoid polluting the entries
@@ -481,18 +481,18 @@ auto SourceTreeService::CommonImportToGit(
     // wrap logger for GitRepo call
     std::string err;
     auto wrapped_logger = std::make_shared<GitRepo::anon_logger_t>(
-        [content_path, repo_path, &err](auto const& msg, bool fatal) {
+        [&root_path, &repo_path, &err](auto const& msg, bool fatal) {
             if (fatal) {
                 err = fmt::format(
                     "While committing directory {} in repository {}:\n{}",
-                    content_path.string(),
+                    root_path.string(),
                     repo_path.string(),
                     msg);
             }
         });
     // stage and commit all
     auto commit_hash =
-        git_repo->CommitDirectory(content_path, commit_message, wrapped_logger);
+        git_repo->CommitDirectory(root_path, commit_message, wrapped_logger);
     if (not commit_hash) {
         return unexpected{err};
     }
diff --git a/src/buildtool/storage/local_cas.tpp b/src/buildtool/storage/local_cas.tpp
index 7db6be3d..a7f81b1a 100644
--- a/src/buildtool/storage/local_cas.tpp
+++ b/src/buildtool/storage/local_cas.tpp
@@ -325,7 +325,7 @@ auto LocalCAS<kDoGlobalUplink>::CheckTreeInvariant(
             auto const digest =
                 ArtifactDigestFactory::Create(hash_function_.GetType(),
                                               ToHexString(entry.first),
-                                              /*size_unknown=*/0,
+                                              0,  // size unknown
                                               IsTreeObject(item.type));
             if (not digest) {
                 return LargeObjectError{
diff --git a/test/buildtool/build_engine/target_map/target_map.test.cpp b/test/buildtool/build_engine/target_map/target_map.test.cpp
index 99fe1d4e..9f66fd92 100644
--- a/test/buildtool/build_engine/target_map/target_map.test.cpp
+++ b/test/buildtool/build_engine/target_map/target_map.test.cpp
@@ -86,7 +86,7 @@ auto SetupConfig() -> RepositoryConfig {
 
 }  // namespace
 
-TEST_CASE("simple targets", "[target_map]") {
+TEST_CASE("simple targets", "[target_map]") {  // NOLINT
     auto const storage_config = TestStorageConfig::Create();
     auto const storage = Storage::Create(&storage_config.Get());
 
diff --git a/test/buildtool/execution_api/bazel/bazel_msg_factory.test.cpp b/test/buildtool/execution_api/bazel/bazel_msg_factory.test.cpp
index 5262971c..ca9d3ca9 100644
--- a/test/buildtool/execution_api/bazel/bazel_msg_factory.test.cpp
+++ b/test/buildtool/execution_api/bazel/bazel_msg_factory.test.cpp
@@ -32,7 +32,7 @@
 namespace {
 /// \brief Create a blob from the content found in file or symlink pointed to by
 /// given path.
-[[nodiscard]] static inline auto CreateBlobFromPath(
+[[nodiscard]] inline auto CreateBlobFromPath(
     std::filesystem::path const& fpath,
     HashFunction hash_function) noexcept -> std::optional<ArtifactBlob> {
     auto const type = FileSystemManager::Type(fpath, /*allow_upwards=*/true);