carverauto/serviceradar

Fork 0

002 build bazel integration #2233

Merged

mfreeman451 merged 41 commits from refs/pull/2233/head into main

2025-09-23 16:18:19 +00:00

mfreeman451 commented

2025-09-23 16:18:09 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #1653
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/1653
Original created: 2025-09-23T16:18:09Z
Original updated: 2025-09-23T16:20:15Z
Original head: carverauto/serviceradar:002-build-bazel-integration
Original base: main
Original merged: 2025-09-23T16:18:19Z by @mfreeman451

PR Type

Enhancement

Description

Integrate Bazel build system with comprehensive CI/CD setup
Fix OCaml toolchain and opam sandboxing issues
Add BuildBuddy remote execution and caching configuration
Improve test infrastructure with better privilege handling

Diagram Walkthrough

flowchart LR
  A["Bazel Integration"] --> B["CI/CD Setup"]
  A --> C["OCaml Toolchain Fixes"]
  A --> D["Remote Execution"]
  B --> E["GitHub Actions"]
  C --> F["Opam Sandboxing"]
  C --> G["Compiler Flags"]
  D --> H["BuildBuddy"]

File Walkthrough

Relevant files

Enhancement

3 files

main.yml `Add comprehensive CI workflow with Bazel`	+43/-0
obazl.opamrc `Improve sandbox fallback handling`	+34/-1
proton_client.mli `Add parameter substitution helper function`	+4/-0

Miscellaneous

1 files

bazel-bootstrap.yml
Remove old bootstrap workflow +0/-36

Configuration changes

2 files

.bazelrc `Configure remote execution and compiler flags`	+11/-0
bazel `Skip Bazelisk wrapper for direct execution`	+1/-1

Dependencies

1 files

MODULE.bazel
Update dependencies and add Java patches +8/-3

Bug fix

5 files

opam.bzl `Fix OCaml toolchain with sandboxing and compiler flags`	+59/-12
opam_ops.bzl `Disable opam sandboxing for CI compatibility`	+34/-20
opam_toolchain_xdg.bzl `Add override repository and sandboxing fixes`	+121/-25
openbsd_jni_header.patch `Fix OpenBSD JNI header compatibility`	+32/-0
ext_emit_build_bazel.c `Fix buffer overflow in C code`	+1/-1

Tests

5 files

sweep_performance_test.go `Improve test privilege checking and mock setup`	+20/-9
syn_scanner_test.go `Add better permission error handling`	+26/-8
tcp_optimization_test.go `Fix test mocking and privilege checks`	+25/-3
tests.rs `Add Bazel runfiles path resolution`	+38/-3
BUILD.bazel `Tag OCaml tests for filtering`	+4/-0

Additional files

13 files

srql-translator.opam	+2/-2
BUILD.bazel	+1/-0
0001-Fix-use-after-free-in-findlibc.patch	+40/-0
BUILD.bazel	+1/-6
openssl_src_runfiles_patch	[link]
BUILD.bazel	+2/-2
opam_dep.bzl	+11/-6
opam_repo.bzl	+9/-3
opam_toolchain_global.bzl	+8/-7
opam_toolchain_local.bzl	+38/-22
opam_toolchain_opam.bzl	+5/-4
opam	+36/-0
repo	+1/-0

Imported from GitHub pull request. Original GitHub pull request: #1653 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/1653 Original created: 2025-09-23T16:18:09Z Original updated: 2025-09-23T16:20:15Z Original head: carverauto/serviceradar:002-build-bazel-integration Original base: main Original merged: 2025-09-23T16:18:19Z by @mfreeman451 --- ### **PR Type** Enhancement ___ ### **Description** - Integrate Bazel build system with comprehensive CI/CD setup - Fix OCaml toolchain and opam sandboxing issues - Add BuildBuddy remote execution and caching configuration - Improve test infrastructure with better privilege handling ___ ### Diagram Walkthrough ```mermaid flowchart LR A["Bazel Integration"] --> B["CI/CD Setup"] A --> C["OCaml Toolchain Fixes"] A --> D["Remote Execution"] B --> E["GitHub Actions"] C --> F["Opam Sandboxing"] C --> G["Compiler Flags"] D --> H["BuildBuddy"] ``` <details> <summary><h3> File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Enhancement</strong></td><td><details><summary>3 files</summary><table> <tr> <td><strong>main.yml</strong><dd><code>Add comprehensive CI workflow with Bazel</code>                                  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1653/files#diff-7829468e86c1cc5d5133195b5cb48e1ff6c75e3e9203777f6b2e379d9e4882b3">+43/-0</a>    </td> </tr> <tr> <td><strong>obazl.opamrc</strong><dd><code>Improve sandbox fallback handling</code>                                                </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1653/files#diff-8a3f2ef5fa2d4ad1c1d9b68f012486ab41ce8a898044f94d046f5b35dc133d59">+34/-1</a>    </td> </tr> <tr> <td><strong>proton_client.mli</strong><dd><code>Add parameter substitution helper function</code>                              </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1653/files#diff-2b549609ea8b875de4a039282ea09be7655ebda3f27f078ea603b8e77b9e31c6">+4/-0</a>      </td> </tr> </table></details></td></tr><tr><td><strong>Miscellaneous</strong></td><td><details><summary>1 files</summary><table> <tr> <td><strong>bazel-bootstrap.yml</strong><dd><code>Remove old bootstrap workflow</code>                                                        </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1653/files#diff-fab69e9bf6a64c8cd4fab4e300f193db8477bda6f7561af140337c000e904528">+0/-36</a>    </td> </tr> </table></details></td></tr><tr><td><strong>Configuration changes</strong></td><td><details><summary>2 files</summary><table> <tr> <td><strong>.bazelrc</strong><dd><code>Configure remote execution and compiler flags</code>                        </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1653/files#diff-544556920c45b42cbfe40159b082ce8af6bd929e492d076769226265f215832f">+11/-0</a>    </td> </tr> <tr> <td><strong>bazel</strong><dd><code>Skip Bazelisk wrapper for direct execution</code>                              </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1653/files#diff-a93512a96f59705d6d94444b257a757158130b0dac6828a8dc77900fed2f87f9">+1/-1</a>      </td> </tr> </table></details></td></tr><tr><td><strong>Dependencies</strong></td><td><details><summary>1 files</summary><table> <tr> <td><strong>MODULE.bazel</strong><dd><code>Update dependencies and add Java patches</code>                                  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1653/files#diff-6136fc12446089c3db7360e923203dd114b6a1466252e71667c6791c20fe6bdc">+8/-3</a>      </td> </tr> </table></details></td></tr><tr><td><strong>Bug fix</strong></td><td><details><summary>5 files</summary><table> <tr> <td><strong>opam.bzl</strong><dd><code>Fix OCaml toolchain with sandboxing and compiler flags</code>      </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1653/files#diff-bda22f3d4ab343b060ce7002755e369a872b8f14a7b9968cf5750f511fcc8f97">+59/-12</a>  </td> </tr> <tr> <td><strong>opam_ops.bzl</strong><dd><code>Disable opam sandboxing for CI compatibility</code>                          </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1653/files#diff-373e06026eb380acb27a454063bea5e79f0de1e7219d17a914ba74c70168d36b">+34/-20</a>  </td> </tr> <tr> <td><strong>opam_toolchain_xdg.bzl</strong><dd><code>Add override repository and sandboxing fixes</code>                          </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1653/files#diff-4bbd6115769a5687c3628c9ec83f90c484f2c92edf002e7264c4cc17cbac052b">+121/-25</a></td> </tr> <tr> <td><strong>openbsd_jni_header.patch</strong><dd><code>Fix OpenBSD JNI header compatibility</code>                                          </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1653/files#diff-0a8b5bec8f076e8e7b1350ef429ec7adde110eed6942e55fb36515cdc39e0991">+32/-0</a>    </td> </tr> <tr> <td><strong>ext_emit_build_bazel.c</strong><dd><code>Fix buffer overflow in C code</code>                                                        </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1653/files#diff-96dec2b952a9257baf70027d3bbd9bff07cf487110b06999d38847e3bbf3b577">+1/-1</a>      </td> </tr> </table></details></td></tr><tr><td><strong>Tests</strong></td><td><details><summary>5 files</summary><table> <tr> <td><strong>sweep_performance_test.go</strong><dd><code>Improve test privilege checking and mock setup</code>                      </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1653/files#diff-77b4e4256ad3aa348ecec7e2548e00f7b23cdaccde4322f7ed18be4cc7d5148d">+20/-9</a>    </td> </tr> <tr> <td><strong>syn_scanner_test.go</strong><dd><code>Add better permission error handling</code>                                          </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1653/files#diff-4baf65f9777013bd39bcea999747bda5c548793454c3cc3a69a67134a6490637">+26/-8</a>    </td> </tr> <tr> <td><strong>tcp_optimization_test.go</strong><dd><code>Fix test mocking and privilege checks</code>                                        </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1653/files#diff-1218aed38178a35178132b397803ada99d736109eacf734a56014f77631ea076">+25/-3</a>    </td> </tr> <tr> <td><strong>tests.rs</strong><dd><code>Add Bazel runfiles path resolution</code>                                              </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1653/files#diff-85c5601521c9a9784723e5882651a72b3828419842d17046e71c65330bb95419">+38/-3</a>    </td> </tr> <tr> <td><strong>BUILD.bazel</strong><dd><code>Tag OCaml tests for filtering</code>                                                        </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1653/files#diff-3cf19e3c1427984c9b33005d104936b2a2d5bcece3509bfe67a4d6aa0c1205b4">+4/-0</a>      </td> </tr> </table></details></td></tr><tr><td><strong>Additional files</strong></td><td><details><summary>13 files</summary><table> <tr> <td><strong>srql-translator.opam</strong></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1653/files#diff-900d10104207edf7fbf525b51ccffa5711742688c181ed29a41cabd9ca1579c2">+2/-2</a>      </td> </tr> <tr> <td><strong>BUILD.bazel</strong></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1653/files#diff-973b449612421d56993c00a7954048b7036f9a8062e462a39552e44f456798fb">+1/-0</a>      </td> </tr> <tr> <td><strong>0001-Fix-use-after-free-in-findlibc.patch</strong></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1653/files#diff-9b2f09d4d345cf1fd275e458786eb76dbd387e6d836f5005fc4fb3c2863580e8">+40/-0</a>    </td> </tr> <tr> <td><strong>BUILD.bazel</strong></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1653/files#diff-b11e6fe1253aecdf8eb31089dfbad9c2a9cf0838e5f22b350611d1c595c0a060">+1/-6</a>      </td> </tr> <tr> <td><strong>openssl_src_runfiles_patch</strong></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1653/files#diff-989250a0876699031fcb92a47bd51b7af1d1bee3da4d34f95cbc40d06b5a5322">[link]</a>    </td> </tr> <tr> <td><strong>BUILD.bazel</strong></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1653/files#diff-6c487b380bd8ffa8f7f7b9f82b688302d9f5775e365cd965c16a0320b8b85cf2">+2/-2</a>      </td> </tr> <tr> <td><strong>opam_dep.bzl</strong></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1653/files#diff-a619985e66e32c4dc40d9c46234b274720f60454f473d7ad453992dc3bc710ff">+11/-6</a>    </td> </tr> <tr> <td><strong>opam_repo.bzl</strong></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1653/files#diff-43e0d7b7d6c564baa6b87742795aaeb71c161fc53ec93921f36fa9fd7579aacb">+9/-3</a>      </td> </tr> <tr> <td><strong>opam_toolchain_global.bzl</strong></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1653/files#diff-6f82fc5f02cd18e1c0252fe312a66d1b0b5f6c4c342d0d44bca3a0a020ae533e">+8/-7</a>      </td> </tr> <tr> <td><strong>opam_toolchain_local.bzl</strong></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1653/files#diff-ebde32b9ccc967deecdeb871b5bdf42d7875130ea1c4739080d1a4bd0d0c0da8">+38/-22</a>  </td> </tr> <tr> <td><strong>opam_toolchain_opam.bzl</strong></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1653/files#diff-31bd6bc6d52eb0d3b0c91e322357730cf82c7d4518d2b0c23f09b8c18b6d5e25">+5/-4</a>      </td> </tr> <tr> <td><strong>opam</strong></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1653/files#diff-02f3996a811c0fa929a15b23c7488f3a7e3ed47e2f38e1d901b4c2bf1d7fd1e0">+36/-0</a>    </td> </tr> <tr> <td><strong>repo</strong></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1653/files#diff-518559990147ca1efa2c553ca37135bcbefd413ffe712590f849cf9d2c65297a">+1/-0</a>      </td> </tr> </table></details></td></tr></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2025-09-23 16:18:56 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1653#issuecomment-3324711282
Original created: 2025-09-23T16:18:56Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 PR contains tests

🔒 Security concerns

Sensitive information exposure:
Ensure the BuildBuddy API key is only used via GitHub Secrets and not leaked in logs. The workflow writes the API key into .bazelrc.remote; verify that CI logs do not echo the header line. Also, sandbox fallback in sandbox.sh executes commands without isolation when bwrap is unavailable; while necessary for CI, confirm that this behavior is restricted to trusted environments and cannot be triggered in untrusted builds.

⚡ Recommended focus areas for review

Flaky Test Risk

The new permissionError-based skipping logic may still pass through non-permission errors to require.NoError, potentially causing intermittent failures on environments with partial raw socket support. Consider tightening the skip conditions or adding more explicit capability checks.

func permissionError(err error) bool {
	if err == nil {
		return false
	}

	return errors.Is(err, syscall.EPERM) ||
		errors.Is(err, syscall.EACCES) ||
		strings.Contains(err.Error(), "requires root")
}

func TestNewSYNScanner(t *testing.T) {
	if testing.Short() {
		t.Skip("Skipping SYN scanner test in short mode")
	}

	// Check if running as root
	isRoot := os.Geteuid() == 0
	tests := []struct {
		name        string
		timeout     time.Duration
		concurrency int
		wantTimeout time.Duration
		wantConc    int
	}{
		{
			name:        "default values",
			timeout:     0,
			concurrency: 0,
			wantTimeout: 1 * time.Second,
			wantConc:    256,
		},
		{
			name:        "custom values",
			timeout:     500 * time.Millisecond,
			concurrency: 100,
			wantTimeout: 500 * time.Millisecond,
			wantConc:    100,
		},
	}

	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			log := logger.NewTestLogger()
			scanner, err := NewSYNScanner(tt.timeout, tt.concurrency, log, nil)

			if err != nil {
				if !isRoot {
					require.Error(t, err)
					assert.Nil(t, scanner)
					t.Logf("SYN scanner correctly failed without root privileges: %v", err)

					return
				}

				if permissionError(err) {
					t.Skipf("Skipping SYN scanner test: raw sockets unavailable (%v)", err)
				}

				require.NoError(t, err)
			}

Platform Toolchain Selection

The selection of --extra_toolchains based on mctx.os.name/arch may not cover all Linux arches and relies on local_config_cc; ensure these labels exist across CI runners and that BAZEL_DO_NOT_DETECT_CPP_TOOLCHAIN interactions won’t cause inconsistent toolchain resolution.

cmd.extend(shared_flags)

toolchain_flag = None
if mctx.os.name == "mac os x":
    toolchain_flag = "@local_config_cc_toolchains//:cc-toolchain-darwin_arm64"
elif mctx.os.name == "linux":
    if mctx.os.arch in ("x86_64", "amd64"):
        toolchain_flag = "@local_config_cc_toolchains//:cc-toolchain-k8"
    elif mctx.os.arch in ("aarch64", "aarch64_be"):
        toolchain_flag = "@local_config_cc_toolchains//:cc-toolchain-aarch64"

disable_local_cc = mctx.getenv("BAZEL_DO_NOT_DETECT_CPP_TOOLCHAIN")
if disable_local_cc and disable_local_cc != "0":
    toolchain_flag = None

if toolchain_flag:
    cmd.append("--extra_toolchains={}".format(toolchain_flag))
if verbosity > 1:
    cmd.append("--subcommands=pretty_print")

mctx.report_progress("Building @tools_opam//extensions/config")
# print("\nRunning cfg tool build:\n%s" % cmd)
res = mctx.execute(
    cmd,
    environment = {
        "HOME": mctx.getenv("HOME") + "/.cache",
        "XDG_CACHE_HOME": mctx.getenv("HOME") + "/.cache",
        "BAZEL_DO_NOT_DETECT_CPP_TOOLCHAIN": "0",
        "OBAZL_NO_BWRAP": "1",
    },
    quiet = (verbosity < 1),
)

Missing Secret Handling

Remote cache setup writes .bazelrc.remote only if BUILDBUDDY_ORG_API_KEY is set, but build/test always pass --config=ci which uses --config=remote; ensure CI doesn’t hang or error when remote is configured without credentials and that fallbacks are in place.

name: CI

on:
  push:
    branches:
      - main
  pull_request:

jobs:
  build:
    runs-on: ubuntu-latest
    permissions:
      contents: read
    env:
      BUILDBUDDY_ORG_API_KEY: ${{ secrets.BUILDBUDDY_ORG_API_KEY }}

    steps:
      - name: Checkout
        uses: actions/checkout@v4

      - name: Install dependencies
        run: |
          sudo apt-get update
          sudo apt-get install -y bubblewrap

      - name: Install Bazel
        uses: bazel-contrib/setup-bazel@0.15.0
        with:
          bazelisk-version: 1.x

      - name: Configure remote cache
        if: ${{ env.BUILDBUDDY_ORG_API_KEY != '' }}
        run: |
          cat <<EOF > .bazelrc.remote
          common --remote_header=x-buildbuddy-api-key=${BUILDBUDDY_ORG_API_KEY}
          EOF

      - name: Test
        run: |
          bazel test \
            --config=ci \
            --test_tag_filters=-manual,-ocaml \
            //...

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1653#issuecomment-3324711282 Original created: 2025-09-23T16:18:56Z --- ## PR Reviewer Guide 🔍 Here are some key observations to aid the review process: <table> <tr><td>⏱️ <strong>Estimated effort to review</strong>: 4 🔵🔵🔵🔵⚪</td></tr> <tr><td>🧪 <strong>PR contains tests</strong></td></tr> <tr><td>🔒 <strong>Security concerns</strong><br><br> <strong>Sensitive information exposure:</strong><br> Ensure the BuildBuddy API key is only used via GitHub Secrets and not leaked in logs. The workflow writes the API key into .bazelrc.remote; verify that CI logs do not echo the header line. Also, sandbox fallback in sandbox.sh executes commands without isolation when bwrap is unavailable; while necessary for CI, confirm that this behavior is restricted to trusted environments and cannot be triggered in untrusted builds.</td></tr> <tr><td>⚡ <strong>Recommended focus areas for review</strong><br><br> <details><summary><a href='https://github.com/carverauto/serviceradar/pull/1653/files#diff-4baf65f9777013bd39bcea999747bda5c548793454c3cc3a69a67134a6490637R38-R97'><strong>Flaky Test Risk</strong></a> The new permissionError-based skipping logic may still pass through non-permission errors to require.NoError, potentially causing intermittent failures on environments with partial raw socket support. Consider tightening the skip conditions or adding more explicit capability checks. </summary> ```go func permissionError(err error) bool { if err == nil { return false } return errors.Is(err, syscall.EPERM) || errors.Is(err, syscall.EACCES) || strings.Contains(err.Error(), "requires root") } func TestNewSYNScanner(t *testing.T) { if testing.Short() { t.Skip("Skipping SYN scanner test in short mode") } // Check if running as root isRoot := os.Geteuid() == 0 tests := []struct { name string timeout time.Duration concurrency int wantTimeout time.Duration wantConc int }{ { name: "default values", timeout: 0, concurrency: 0, wantTimeout: 1 * time.Second, wantConc: 256, }, { name: "custom values", timeout: 500 * time.Millisecond, concurrency: 100, wantTimeout: 500 * time.Millisecond, wantConc: 100, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { log := logger.NewTestLogger() scanner, err := NewSYNScanner(tt.timeout, tt.concurrency, log, nil) if err != nil { if !isRoot { require.Error(t, err) assert.Nil(t, scanner) t.Logf("SYN scanner correctly failed without root privileges: %v", err) return } if permissionError(err) { t.Skipf("Skipping SYN scanner test: raw sockets unavailable (%v)", err) } require.NoError(t, err) } ``` </details> <details><summary><a href='https://github.com/carverauto/serviceradar/pull/1653/files#diff-bda22f3d4ab343b060ce7002755e369a872b8f14a7b9968cf5750f511fcc8f97R112-R143'><strong>Platform Toolchain Selection</strong></a> The selection of --extra_toolchains based on mctx.os.name/arch may not cover all Linux arches and relies on local_config_cc; ensure these labels exist across CI runners and that BAZEL_DO_NOT_DETECT_CPP_TOOLCHAIN interactions won’t cause inconsistent toolchain resolution. </summary> ```python cmd.extend(shared_flags) toolchain_flag = None if mctx.os.name == "mac os x": toolchain_flag = "@local_config_cc_toolchains//:cc-toolchain-darwin_arm64" elif mctx.os.name == "linux": if mctx.os.arch in ("x86_64", "amd64"): toolchain_flag = "@local_config_cc_toolchains//:cc-toolchain-k8" elif mctx.os.arch in ("aarch64", "aarch64_be"): toolchain_flag = "@local_config_cc_toolchains//:cc-toolchain-aarch64" disable_local_cc = mctx.getenv("BAZEL_DO_NOT_DETECT_CPP_TOOLCHAIN") if disable_local_cc and disable_local_cc != "0": toolchain_flag = None if toolchain_flag: cmd.append("--extra_toolchains={}".format(toolchain_flag)) if verbosity > 1: cmd.append("--subcommands=pretty_print") mctx.report_progress("Building @tools_opam//extensions/config") # print("\nRunning cfg tool build:\n%s" % cmd) res = mctx.execute( cmd, environment = { "HOME": mctx.getenv("HOME") + "/.cache", "XDG_CACHE_HOME": mctx.getenv("HOME") + "/.cache", "BAZEL_DO_NOT_DETECT_CPP_TOOLCHAIN": "0", "OBAZL_NO_BWRAP": "1", }, quiet = (verbosity < 1), ) ``` </details> <details><summary><a href='https://github.com/carverauto/serviceradar/pull/1653/files#diff-7829468e86c1cc5d5133195b5cb48e1ff6c75e3e9203777f6b2e379d9e4882b3R1-R43'><strong>Missing Secret Handling</strong></a> Remote cache setup writes .bazelrc.remote only if BUILDBUDDY_ORG_API_KEY is set, but build/test always pass --config=ci which uses --config=remote; ensure CI doesn’t hang or error when remote is configured without credentials and that fallbacks are in place. </summary> ```yaml name: CI on: push: branches: - main pull_request: jobs: build: runs-on: ubuntu-latest permissions: contents: read env: BUILDBUDDY_ORG_API_KEY: ${{ secrets.BUILDBUDDY_ORG_API_KEY }} steps: - name: Checkout uses: actions/checkout@v4 - name: Install dependencies run: | sudo apt-get update sudo apt-get install -y bubblewrap - name: Install Bazel uses: bazel-contrib/setup-bazel@0.15.0 with: bazelisk-version: 1.x - name: Configure remote cache if: ${{ env.BUILDBUDDY_ORG_API_KEY != '' }} run: | cat <<EOF > .bazelrc.remote common --remote_header=x-buildbuddy-api-key=${BUILDBUDDY_ORG_API_KEY} EOF - name: Test run: | bazel test \ --config=ci \ --test_tag_filters=-manual,-ocaml \ //... ``` </details> </td></tr> </table>

qodo-code-review[bot] commented

2025-09-23 16:20:15 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1653#issuecomment-3324715897
Original created: 2025-09-23T16:20:15Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	OCaml tests are disabled in CI The new CI workflow in the PR disables all OCaml tests by filtering them out with a tag. Instead of skipping them, the underlying toolchain issues causing this should be fixed to ensure proper test coverage. Examples: .github/workflows/main.yml [40-43] `bazel test \ --config=ci \ --test_tag_filters=-manual,-ocaml \ //...` ocaml/srql/BUILD.bazel [208] `tags = ["ocaml"],` Solution Walkthrough: Before: `# .github/workflows/main.yml jobs: build: steps: - name: Test run: \| bazel test \ --config=ci \ --test_tag_filters=-manual,-ocaml \ //... # ocaml/srql/BUILD.bazel ocaml_test( name = "test_query_engine", ... tags = ["ocaml"], )` After: `# .github/workflows/main.yml jobs: build: steps: - name: Test run: \| # Assumes underlying OCaml toolchain issues on Linux are resolved bazel test \ --config=ci \ --test_tag_filters=-manual \ //... # ocaml/srql/BUILD.bazel ocaml_test( name = "test_query_engine", ... # No 'ocaml' tag needed if tests are not filtered out )` Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies that the new CI setup explicitly disables all OCaml tests, which is a significant regression in test coverage that should be addressed rather than bypassed.	High
Possible issue	Fix sandbox fallback logic Improve the sandbox script's fallback logic by executing the command without the sandbox if `mktemp` fails, preventing build failures in constrained environments. third_party/vendor/tools_opam/extensions/obazl.opamrc [271] fallback_log=$(mktemp 2>/dev/null \|\| echo "") if [ -n "$fallback_log" ]; then set +e bwrap "${ARGS[@]}" "$@" 2> >(tee "$fallback_log" >&2) status=$? set -e if [ $status -eq 0 ]; then rm -f "$fallback_log" exit 0 fi if sed -E 's/\x1B\[[0-9;:][@-~]//g' "$fallback_log" \| \ tr -d '\r' \| \ grep -Eq 'bwrap: . (RTM_NEWADDR\|Operation not permitted\|Permission denied)'; then echo "sandbox.sh: bubblewrap unavailable (status $status); running command without sandbox." >&2 rm -f "$fallback_log" exec "$@" fi rm -f "$fallback_log" exit $status else - exec bwrap "${ARGS[@]}" "$@" + # Fallback if mktemp fails: run without sandbox + exec "$@" fi `[To ensure code accuracy, apply this suggestion manually]` Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a flaw in the fallback logic for when `mktemp` fails and proposes a more robust alternative that improves the script's resilience in constrained environments.	Medium
More

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1653#issuecomment-3324715897 Original created: 2025-09-23T16:20:15Z --- ## PR Code Suggestions ✨  Explore these optional code suggestions: <table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion                                                                                                                                    </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=1>High-level</td> <td> <details><summary>OCaml tests are disabled in CI</summary> ___ **The new CI workflow in the PR disables all OCaml tests by filtering them out <br>with a tag. Instead of skipping them, the underlying toolchain issues causing <br>this should be fixed to ensure proper test coverage.** ### Examples: <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/1653/files#diff-7829468e86c1cc5d5133195b5cb48e1ff6c75e3e9203777f6b2e379d9e4882b3R40-R43">.github/workflows/main.yml [40-43]</a> </summary> ```yaml bazel test \ --config=ci \ --test_tag_filters=-manual,-ocaml \ //... ``` </details> <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/1653/files#diff-3cf19e3c1427984c9b33005d104936b2a2d5bcece3509bfe67a4d6aa0c1205b4R208-R208">ocaml/srql/BUILD.bazel [208]</a> </summary> ```starlark tags = ["ocaml"], ``` </details> ### Solution Walkthrough: #### Before: ```starlark # .github/workflows/main.yml jobs: build: steps: - name: Test run: | bazel test \ --config=ci \ --test_tag_filters=-manual,-ocaml \ //... # ocaml/srql/BUILD.bazel ocaml_test( name = "test_query_engine", ... tags = ["ocaml"], ) ``` #### After: ```starlark # .github/workflows/main.yml jobs: build: steps: - name: Test run: | # Assumes underlying OCaml toolchain issues on Linux are resolved bazel test \ --config=ci \ --test_tag_filters=-manual \ //... # ocaml/srql/BUILD.bazel ocaml_test( name = "test_query_engine", ... # No 'ocaml' tag needed if tests are not filtered out ) ``` <details><summary>Suggestion importance[1-10]: 9</summary> __ Why: The suggestion correctly identifies that the new CI setup explicitly disables all OCaml tests, which is a significant regression in test coverage that should be addressed rather than bypassed. </details></details></td><td align=center>High </td></tr><tr><td rowspan=1>Possible issue</td> <td> <details><summary>Fix sandbox fallback logic</summary> ___ **Improve the sandbox script's fallback logic by executing the command without the <br>sandbox if <code>mktemp</code> fails, preventing build failures in constrained environments.** [third_party/vendor/tools_opam/extensions/obazl.opamrc [271]](https://github.com/carverauto/serviceradar/pull/1653/files#diff-8a3f2ef5fa2d4ad1c1d9b68f012486ab41ce8a898044f94d046f5b35dc133d59R271-R271) ```diff fallback_log=$(mktemp 2>/dev/null || echo "") if [ -n "$fallback_log" ]; then set +e bwrap "${ARGS[@]}" "$@" 2> >(tee "$fallback_log" >&2) status=$? set -e if [ $status -eq 0 ]; then rm -f "$fallback_log" exit 0 fi if sed -E 's/\x1B\[[0-9;:]*[@-~]//g' "$fallback_log" | \ tr -d '\r' | \ grep -Eq 'bwrap: .* (RTM_NEWADDR|Operation not permitted|Permission denied)'; then echo "sandbox.sh: bubblewrap unavailable (status $status); running command without sandbox." >&2 rm -f "$fallback_log" exec "$@" fi rm -f "$fallback_log" exit $status else - exec bwrap "${ARGS[@]}" "$@" + # Fallback if mktemp fails: run without sandbox + exec "$@" fi ``` `[To ensure code accuracy, apply this suggestion manually]` <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly identifies a flaw in the fallback logic for when `mktemp` fails and proposes a more robust alternative that improves the script's resilience in constrained environments. </details></details></td><td align=center>Medium </td></tr> <tr><td align="center" colspan="2"> - [ ] More  </td><td></td></tr></tbody></table>