From 6c21f63c966710c388f1ae9f5f2bf55ab1d2fd54 Mon Sep 17 00:00:00 2001 From: ethernet Date: Fri, 31 Jul 2026 02:14:35 -0400 Subject: [PATCH] =?UTF-8?q?fix(ci):=20review=20fixes=20=E2=80=94=20PR-read?= =?UTF-8?q?-only=20buildx=20cache,=20per-arch=20profile=20labels?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review findings on the ARC migration: - docker.yml: WIF auth (and therefore Artifact Registry cache WRITES) now only runs on non-PR events. The build job runs PR-controlled code and the publish job reads the same buildcache ref, so a PR-writable cache was a layer-poisoning vector. PRs of any origin keep cache READS via the runner pod's GKE Workload Identity — that's where the 15min -> 2-3min win comes from; main pushes repopulate writes. - docker.yml: profile label is now docker-tests-. Both matrix legs uploaded resource-profile-docker-tests; upload-artifact v4+ rejects the duplicate and continue-on-error swallowed it, silently dropping one arch's profile. - actions/profile: run the wrapped command with bash -eo pipefail to match normal `run:` step semantics (a failing `source .venv/...` must fail the step, not fall through). - js/e2e/site workflows: bake node22 into the node_modules cache keys so a future node-version bump can't restore stale native builds (node-pty, electron postinstall) against an unchanged lockfile. - test_container_restart_stale_pid: forward deadline_s/interval_s to wait_for_log instead of silently dropping them. - doctor.py: refresh a stale comment on the in-container docker branch. --- .github/actions/profile/action.yml | 5 +++- .github/workflows/deploy-site.yml | 4 +++- .github/workflows/docker.yml | 24 ++++++++++--------- .github/workflows/docs-site-checks.yml | 4 +++- .github/workflows/e2e-desktop.yml | 4 +++- .github/workflows/js-autofix.yml | 3 ++- .github/workflows/js-tests.yml | 7 ++++-- hermes_cli/doctor.py | 5 +++- .../test_container_restart_stale_pid.py | 8 ++++++- 9 files changed, 44 insertions(+), 20 deletions(-) diff --git a/.github/actions/profile/action.yml b/.github/actions/profile/action.yml index e3e0f9b84e8..0cd5ea9d0b0 100644 --- a/.github/actions/profile/action.yml +++ b/.github/actions/profile/action.yml @@ -37,7 +37,10 @@ runs: env: _CMD: ${{ inputs.command }} run: | - bash -c "$_CMD" + # -e / pipefail: match the semantics of a normal `run:` step + # (bash -e {0}) so a failing early line (e.g. `source .venv/...`) + # fails the step instead of silently running the rest. + bash -eo pipefail -c "$_CMD" - name: Stop profiler and collect results id: stop-profiler diff --git a/.github/workflows/deploy-site.yml b/.github/workflows/deploy-site.yml index 3d5ba1878c9..0bf40ab4852 100644 --- a/.github/workflows/deploy-site.yml +++ b/.github/workflows/deploy-site.yml @@ -71,7 +71,9 @@ jobs: id: npm-cache with: path: website/node_modules - key: node-modules-cache-${{ hashFiles('website/package-lock.json') }} + # node22: native builds + engine checks are node-major-specific; + # keep the key in sync with setup-node's node-version above. + key: node-modules-cache-node22-${{ hashFiles('website/package-lock.json') }} - name: Install website dependencies uses: ./.github/actions/retry diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml index 3606dfba4de..163fc5e0547 100644 --- a/.github/workflows/docker.yml +++ b/.github/workflows/docker.yml @@ -34,15 +34,15 @@ jobs: # Buildx layer cache lives in Artifact Registry (us-central1, same region # as the ARC runners) instead of GitHub's cache CDN. Reads are keyless via # GKE Workload Identity on the runner pods; writes are keyless via GitHub - # OIDC -> GCP WIF (google-github-actions/auth). Fork PRs get neither an - # id-token nor pod WI outside our cluster, so they build cache-cold — - # same behavior type=gha gave them. + # OIDC -> GCP WIF (google-github-actions/auth) and happen ONLY on trusted + # main-push/release contexts — PR builds of any origin are read-only so + # PR-controlled code can never write cache layers the publish job reads. build: if: github.repository == 'NousResearch/hermes-agent' permissions: contents: read - # OIDC token for WIF cache writes. Fork-PR runs receive no id-token; - # the auth step below is skipped for them. + # OIDC token for WIF cache writes — only minted on non-PR events + # (see the gcp-auth step); PR runs stay secret-free. id-token: write strategy: fail-fast: false @@ -76,13 +76,15 @@ jobs: if: steps.buildx.outcome == 'failure' uses: docker/setup-buildx-action@bb05f3f5519dd87d3ba754cc423b652a5edd6d2c # v4.2.0 - # Keyless GCP auth via GitHub OIDC -> WIF. Fork PRs have no id-token - # (permissions are read-only in their context), so this step is - # skipped and the build runs cache-read-only via the pod's GKE - # Workload Identity. + # Keyless GCP auth via GitHub OIDC -> WIF. PR builds (fork or + # same-repo) get NO write token: this job runs PR-controlled code + # and the publish job reads the same cache ref, so a PR-writable + # cache would be a layer-poisoning vector. PRs read the cache via + # the pod's GKE Workload Identity; writes happen only on trusted + # main-push/release contexts. - name: Authenticate to GCP (WIF, cache writes) id: gcp-auth - if: github.event.pull_request.head.repo.fork != true + if: github.event_name != 'pull_request' uses: google-github-actions/auth@7c6bc770dae815cd3e89ee6cdf493a5fab2cc093 # v3.0.0 with: project_id: hermes-agent-github-actions @@ -158,7 +160,7 @@ jobs: # env -i into the parallel runner. uses: ./.github/actions/profile with: - label: docker-tests + label: docker-tests-${{ matrix.arch }} command: HERMES_TEST_WORKERS=8 HERMES_TEST_IMAGE="${{ env.IMAGE_NAME }}:test" scripts/run_tests.sh tests/docker/ --file-timeout 600 # --------------------------------------------------------------------------- diff --git a/.github/workflows/docs-site-checks.yml b/.github/workflows/docs-site-checks.yml index 30b3b1fad80..0cfdd50de2e 100644 --- a/.github/workflows/docs-site-checks.yml +++ b/.github/workflows/docs-site-checks.yml @@ -21,7 +21,9 @@ jobs: id: npm-cache with: path: website/node_modules - key: node-modules-cache-${{ hashFiles('website/package-lock.json') }} + # node22: native builds + engine checks are node-major-specific; + # keep the key in sync with setup-node's node-version above. + key: node-modules-cache-node22-${{ hashFiles('website/package-lock.json') }} - name: Install website dependencies uses: ./.github/actions/retry diff --git a/.github/workflows/e2e-desktop.yml b/.github/workflows/e2e-desktop.yml index 26b9e418384..8d4487ef70a 100644 --- a/.github/workflows/e2e-desktop.yml +++ b/.github/workflows/e2e-desktop.yml @@ -46,7 +46,9 @@ jobs: ui-tui/packages/*/node_modules web/node_modules tests-js/node_modules - key: node-modules-full-${{ runner.arch }}-${{ hashFiles('package-lock.json') }} + # node22 in the key: node-pty/electron native artifacts are + # node-major-specific; sync with setup-node's node-version. + key: node-modules-full-node22-${{ runner.arch }}-${{ hashFiles('package-lock.json') }} # Full npm ci (not --ignore-scripts): electron's postinstall # downloads the binary we launch, and node-pty's native build is diff --git a/.github/workflows/js-autofix.yml b/.github/workflows/js-autofix.yml index ebe442b53a2..a5881be0b0a 100644 --- a/.github/workflows/js-autofix.yml +++ b/.github/workflows/js-autofix.yml @@ -81,7 +81,8 @@ jobs: ui-tui/packages/*/node_modules web/node_modules tests-js/node_modules - key: node-modules-noscripts-${{ runner.arch }}-${{ hashFiles('package-lock.json') }} + # node22 in the key: sync with setup-node's node-version. + key: node-modules-noscripts-node22-${{ runner.arch }}-${{ hashFiles('package-lock.json') }} # --ignore-scripts: eslint only needs TS sources + eslint packages. - uses: ./.github/actions/retry diff --git a/.github/workflows/js-tests.yml b/.github/workflows/js-tests.yml index a032c3fdcfc..0e9eaffc32f 100644 --- a/.github/workflows/js-tests.yml +++ b/.github/workflows/js-tests.yml @@ -32,7 +32,8 @@ jobs: tests-js/node_modules # noscripts flavor: MUST NOT share a key with full `npm ci` # installs (electron postinstall binary, node-pty build). - key: node-modules-noscripts-${{ runner.arch }}-${{ hashFiles('package-lock.json') }} + # node22 in the key: sync with setup-node's node-version. + key: node-modules-noscripts-node22-${{ runner.arch }}-${{ hashFiles('package-lock.json') }} - uses: ./.github/actions/retry with: @@ -92,7 +93,9 @@ jobs: ui-tui/packages/*/node_modules web/node_modules tests-js/node_modules - key: node-modules-full-${{ runner.arch }}-${{ hashFiles('package-lock.json') }} + # node22 in the key: node-pty/electron native artifacts are + # node-major-specific; sync with setup-node's node-version. + key: node-modules-full-node22-${{ runner.arch }}-${{ hashFiles('package-lock.json') }} - uses: ./.github/actions/retry with: diff --git a/hermes_cli/doctor.py b/hermes_cli/doctor.py index 0446f39d7f6..339d6f6bc80 100644 --- a/hermes_cli/doctor.py +++ b/hermes_cli/doctor.py @@ -1758,7 +1758,10 @@ def run_doctor(args): elif _is_termux(): check_info("Docker backend is not available inside Termux (expected on Android)") elif running_in_container: - pass # already explained above + # In-container with a non-docker backend: the info line above only + # prints for the implicit `local` case, but either way a missing + # docker binary inside the container is expected — stay quiet. + pass else: check_warn("docker not found", "(optional)") diff --git a/tests/docker/test_container_restart_stale_pid.py b/tests/docker/test_container_restart_stale_pid.py index 72e15eeda69..84edf9eeb00 100644 --- a/tests/docker/test_container_restart_stale_pid.py +++ b/tests/docker/test_container_restart_stale_pid.py @@ -49,7 +49,13 @@ def _wait_for_reconcile_log_mention( ) -> str: """Poll until /opt/data/logs/container-boot.log mentions `profile`. """ - return wait_for_log(container, "/opt/data/logs/container-boot.log", f"profile={profile}") + return wait_for_log( + container, + "/opt/data/logs/container-boot.log", + f"profile={profile}", + deadline_s=deadline_s, + interval_s=interval_s, + ) @pytest.fixture