Skip to content

发prod测试: merge #911 into latest main#932

Closed
law-chain-hot wants to merge 10 commits into
mainfrom
发prod测试

Hidden character warning

The head ref may contain hidden characters: "\u53d1prod\u6d4b\u8bd5"
Closed

发prod测试: merge #911 into latest main#932
law-chain-hot wants to merge 10 commits into
mainfrom
发prod测试

Conversation

@law-chain-hot

@law-chain-hot law-chain-hot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

What

Release-candidate branch from latest origin/main plus PR #911 (feat/ops-runner-deploy-scripts).

Local verification

  • git diff --check origin/main...HEAD
  • scripts/deploy/tests/parity/run.sh (PARITY: PASS, SSM payload byte-identical)

Notes

Summary by CodeRabbit

  • New Features

    • Added safer, more automated runner build and update workflows, including support for packaged releases and in-place upgrades.
    • Deployment now syncs environment values and secrets from cloud storage for eligible stages.
  • Bug Fixes

    • Improved stage-aware cloud configuration so deployments use the correct region and permissions.
    • Strengthened runner upgrades with verification, rollback handling, and better timeout/error reporting.
  • Chores

    • Updated container builds to use cached dependencies, reducing build times across services.

Fleet re-runs and canary-then-fleet overlaps hit runners that already run the
exact incoming bits. Compare sha256 of the incoming binary against the current
symlink target before creating a release dir: identical -> log + exit 0.
Hash comparison (not version label) keeps the no-op safe when a dev artifact
reuses a version name.
The 'deferred' branch of verify_embedded_runtime_hash passed silently when no
runtime cache dir existed yet — a runner whose embedded guest key could never
materialize installed cleanly and then failed every box start with a hash
mismatch. Now: wait up to 20s for the runner's self-extraction, verify the
guest hash, and return FATAL on a miss so restart_with_target routes to the
existing auto-rollback branch. Better no install than a runner that cannot
start boxes.
…oad)

Root fix for 'Guest binary hash mismatch' on dev artifacts: the build script
now locates the embedded runtime dir BY the fresh guest's sha256 and packs it
into the tarball as boxlite-runtime.tar.gz, so the shipped runtime always
matches the compile-time key. Supersedes the interim same-binary skip and
guest-extraction fuse on these files (to be re-evaluated on top of the new
install flow if still needed).
PATH-shim fixture records the semantic command sequence (cargo/go/make/git/
tar/aws/curl) each implementation issues and byte-compares artifacts,
sidecars, and the base64 SSM payload. Red until the .mjs twins land.
…rity)

Line-for-line port of build-runner-binary.sh: same CLI, same artifact names,
same sidecars, same command sequence (parity-tested). Selected at the
orchestration layer via OPS_RUNNER_IMPL; bash stays the default.
…ority)

Ports the local orchestrator half (region/instance resolution, artifact
staging, S3 presign, SSM dispatch + poll). The remote upgrade payload is not
duplicated: the .mjs parses the heredoc out of the sibling .sh at runtime and
expands it with bash here-document semantics, so the two halves cannot drift.
Parity test proves the SSM payload byte-identical.
Add BuildKit cache mounts (yarn Berry global cache, Go build + module
caches) to the api/otel-collector/proxy/ssh-gateway/runner image builds so
deps and compiled objects persist across builds instead of recompiling from
cold every time. Also add the dockerfile:1 syntax header to the Go images.
Layer ordering, build commands, and outputs are unchanged.
@law-chain-hot law-chain-hot requested a review from a team as a code owner July 6, 2026 12:33
@coderabbitai

coderabbitai Bot commented Jul 6, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This PR adds BuildKit cache mounts to service Dockerfiles, introduces stage-scoped AWS region configuration with SSM-based secrets syncing for infra deployment, and adds new bash/Node.js scripts for building and hot-upgrading the boxlite-runner binary, plus a parity test harness comparing the bash and JS implementations.

Changes

Dockerfile Build Caching

Layer / File(s) Summary
BuildKit cache mount adoption
apps/api/Dockerfile, apps/otel-collector/Dockerfile, apps/proxy/Dockerfile, apps/runner/Dockerfile, apps/ssh-gateway/Dockerfile
Adds syntax directives, extra OS build packages, and cache mounts for Yarn berry cache, Go module cache, and Go build cache across all service Dockerfiles.

Stage-Scoped Infra Configuration

Layer / File(s) Summary
Region and IAM/cache wiring
apps/infra/sst.config.ts
Introduces stage→region mapping, IAM permissionsBoundary transform, and a serviceImageCache flag, wired into VPC endpoints, IAM ARNs, env vars, Docker image cache settings, and runner user-data.
SSM env/secrets sync
apps/infra/scripts/sst-with-cloudflare.mjs
Adds a runAws() wrapper, stage-based region resolution, and a syncProductEnvFromSsm() routine writing .env and SST secrets from SSM before launching sst.

Runner Binary Build and Update Tooling

Layer / File(s) Summary
Build scripts (bash + JS parity)
scripts/deploy/build-runner-binary.sh, scripts/deploy/build-runner-binary.mjs
New scripts build the boxlite-runner tarball with embedded runtime, guest hash, and version metadata outside GitHub Actions.
Update scripts (bash + JS parity)
scripts/deploy/runner-update-binary.sh, scripts/deploy/runner-update-binary.mjs
Rewrites the remote upgrade flow into a versioned release directory with hot-adoption verification and rollback, orchestrated via SSM with explicit completion polling.
Parity test harness
scripts/deploy/tests/parity/run.sh, scripts/deploy/tests/parity/setup-fixture.sh
Adds a fixture generator with command shims and a test runner that diffs bash vs JS build/update call logs, tarball contents, and SSM payloads.

Estimated code review effort: 4 (Complex) | ~75 minutes

Possibly related PRs

  • boxlite-ai/boxlite#730: Both PRs modify apps/api/Dockerfile's multi-stage deps/runtime-deps Yarn installation setup.
  • boxlite-ai/boxlite#793: Both PRs modify runner bootstrap/user-data generation in apps/infra/sst.config.ts.
  • boxlite-ai/boxlite#794: Both PRs change scripts/deploy/runner-update-binary.sh to harden the SSM-based runner upgrade flow.

Suggested reviewers: G4614

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed Title is directly related to the PR: it describes merging #911 onto the latest main for a prod test/release-candidate branch.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch 发prod测试

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@law-chain-hot

Copy link
Copy Markdown
Contributor Author

Closing per release workflow: this is only a branch for prod testing consumption, not a PR.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4aa0877d49

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +32 to +35
DEV_RUNNER_INSTANCE_NAME="${DEV_RUNNER_INSTANCE_NAME:-boxlite-dev-runner-default}"
DEV_RUNNER_INSTANCE_ID="${DEV_RUNNER_INSTANCE_ID:-}"
PROD_AWS_REGION="${PROD_AWS_REGION:-us-west-2}"
PROD_RUNNER_INSTANCE_NAME="${PROD_RUNNER_INSTANCE_NAME:-boxlite-prod-runner-default}"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Use the actual runner Name tag by default

When this script is run with the documented/default inputs, the EC2 lookup filters for tag:Name=boxlite-dev-runner-default or boxlite-prod-runner-default, but the SST stack still tags the default runner as boxlite-runner-default (apps/infra/sst.config.ts:980, also documented in apps/infra/README.md:259). That makes the default describe-instances path find zero instances and abort every runner update unless operators know to override RUNNER_INSTANCE_NAME or RUNNER_INSTANCE_ID.

Useful? React with 👍 / 👎.

Comment on lines +403 to +405
if [ ! -d "\$cache_dir" ]; then
echo "FATAL: runtime cache directory missing before start: \$cache_dir" >&2
return 1

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Let release binaries create their runtime cache before pinning

For official GitHub release updates, the existing release workflow packages only boxlite-runner in the tarball (.github/workflows/build-runner-binary.yml:92), so GUEST_EXPECTED is empty and no runtime payload is installed before restart. This check runs before systemctl start, but the release binary's embedded runtime cache is created only when that new binary starts, so a normal release rollout fails here unless the exact version cache already exists on the host.

Useful? React with 👍 / 👎.


GUEST_EXPECTED=""
RUNTIME_SUFFIX=""
RUNTIME_CACHE_DIR_NAME="v${RUNTIME_CACHE_VERSION}"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Restore the previous runtime cache when rolling back

If an upgrade to a different version fails after the symlink switch, rollback still pins BOXLITE_RUNTIME_DIR to v${RUNTIME_CACHE_VERSION}, which is derived from the failed target version rather than the previous CURRENT_TARGET. For upgrades such as 0.9.7 to 0.9.8, restart_rollback_target "$CURRENT_TARGET" can fail before starting the old binary because the new version cache is missing, or it can run the old binary against the wrong runtime directory.

Useful? React with 👍 / 👎.

@law-chain-hot law-chain-hot deleted the 发prod测试 branch July 6, 2026 12:40

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (1)
apps/infra/scripts/sst-with-cloudflare.mjs (1)

81-87: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Add bounded retry/backoff for AWS throttling.

runAws() is now the shared SSM path, but transient ThrottlingException/rate-limit failures abort deploys immediately. Add a small retry loop for retryable AWS CLI failures. As per coding guidelines, apps/infra/**/*.{py,js,ts,tsx,java,go,rb,php} should “Include error handling for API failures and rate limiting”.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/infra/scripts/sst-with-cloudflare.mjs` around lines 81 - 87, The shared
AWS CLI wrapper runAws() currently fails immediately on transient throttling or
rate-limit errors, so add a small bounded retry loop with backoff around the
execFileSync call. Detect retryable AWS CLI failures such as ThrottlingException
and similar rate-limit messages, retry only a limited number of times with
increasing delay, and preserve the existing env/options behavior and REGION
setup. Keep the change localized to runAws() in sst-with-cloudflare.mjs so the
SSM path benefits automatically.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@apps/infra/scripts/sst-with-cloudflare.mjs`:
- Around line 163-177: The SSM-to-dotenv mapping in records currently derives
keys from only the last path segment, which can create duplicate or invalid .env
entries. Update the records processing in sst-with-cloudflare.mjs to validate
each derived key before writeAtomically runs: reject empty or malformed dotenv
keys and fail fast if two params produce the same key. Use the existing records
pipeline and dotenvLine call site to add this check right after
mapping/filtering, before sorting and writing.

In `@apps/infra/sst.config.ts`:
- Around line 31-32: The permissions boundary ARN is hard-coded to a single
account, which will break deployments in other accounts. Update the
ACCOUNT_ID/IAM_ROLE_BOUNDARY_ARN logic in sst.config.ts to derive the account
from the deployment context or make it configurable via environment/config, and
ensure the IAM_ROLE_BOUNDARY_ARN used by the stack always matches the target
account.

In `@scripts/deploy/runner-update-binary.sh`:
- Around line 571-580: The rollback path in restart_rollback_target is using the
failed upgrade’s runtime cache override instead of the previously active one.
Update restart_rollback_target and restart_with_target handling so rollback
restores the saved pre-upgrade values for GUEST_EXPECTED, RUNTIME_SUFFIX, and
especially RUNTIME_CACHE_DIR_NAME, rather than always forcing
v${RUNTIME_CACHE_VERSION}. Use the existing saved_* locals in
restart_rollback_target or persist the runtime cache name per release target,
then pass that restored value into restart_with_target so the old binary
restarts with its original runtime override.
- Around line 600-611: The runner checksum fallback in the download/install flow
currently continues when the .sha256 file cannot be fetched, which leaves the
privileged binary rollout unverified. Update the checksum handling in
runner-update-binary.sh so the branch around the curl/awk/sha256sum logic fails
closed: if DOWNLOAD_SHA_URL cannot be downloaded, abort the install path with a
fatal error instead of printing the warning and proceeding. Keep the behavior
centered around the existing checksum verification block and its EXPECTED/ACTUAL
comparison.

---

Nitpick comments:
In `@apps/infra/scripts/sst-with-cloudflare.mjs`:
- Around line 81-87: The shared AWS CLI wrapper runAws() currently fails
immediately on transient throttling or rate-limit errors, so add a small bounded
retry loop with backoff around the execFileSync call. Detect retryable AWS CLI
failures such as ThrottlingException and similar rate-limit messages, retry only
a limited number of times with increasing delay, and preserve the existing
env/options behavior and REGION setup. Keep the change localized to runAws() in
sst-with-cloudflare.mjs so the SSM path benefits automatically.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: cdc2bf2d-e69f-4a86-bff8-6dfacdc57740

📥 Commits

Reviewing files that changed from the base of the PR and between 53eb510 and 4aa0877.

📒 Files selected for processing (13)
  • apps/api/Dockerfile
  • apps/infra/scripts/sst-with-cloudflare.mjs
  • apps/infra/sst.config.ts
  • apps/otel-collector/Dockerfile
  • apps/proxy/Dockerfile
  • apps/runner/Dockerfile
  • apps/ssh-gateway/Dockerfile
  • scripts/deploy/build-runner-binary.mjs
  • scripts/deploy/build-runner-binary.sh
  • scripts/deploy/runner-update-binary.mjs
  • scripts/deploy/runner-update-binary.sh
  • scripts/deploy/tests/parity/run.sh
  • scripts/deploy/tests/parity/setup-fixture.sh

Comment on lines +163 to +177
const records = params
.map((param) => ({
key: String(param.Name || '').split('/').filter(Boolean).pop(),
value: String(param.Value ?? ''),
version: Number(param.Version || 0),
}))
.filter((param) => param.key)
.sort((a, b) => a.key.localeCompare(b.key))

if (records.length === 0) {
console.error(`sst-env-sync: no parameters found under ${prefix} in ${REGION}; seed SSM before running SST with BOXLITE_ENV_SOURCE=ssm`)
process.exit(1)
}

writeAtomically(outFile, `${records.map((param) => dotenvLine(param.key, param.value)).join('\n')}\n`)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Reject duplicate or invalid SSM-derived dotenv keys.

Using only the final path segment can silently produce duplicate keys from nested SSM paths, and malformed leaf names can generate invalid .env entries. Fail fast before writing .env.

Suggested validation
   const records = params
     .map((param) => ({
       key: String(param.Name || '').split('/').filter(Boolean).pop(),
       value: String(param.Value ?? ''),
       version: Number(param.Version || 0),
     }))
     .filter((param) => param.key)
     .sort((a, b) => a.key.localeCompare(b.key))
+
+  const invalid = records.filter((param) => !/^[A-Za-z_][A-Za-z0-9_]*$/.test(param.key))
+  if (invalid.length) {
+    console.error(`sst-env-sync: invalid dotenv key(s): ${invalid.map((p) => p.key).join(', ')}`)
+    process.exit(1)
+  }
+
+  const seen = new Set()
+  const duplicates = records.filter((param) => {
+    if (seen.has(param.key)) return true
+    seen.add(param.key)
+    return false
+  })
+  if (duplicates.length) {
+    console.error(`sst-env-sync: duplicate dotenv key(s): ${duplicates.map((p) => p.key).join(', ')}`)
+    process.exit(1)
+  }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const records = params
.map((param) => ({
key: String(param.Name || '').split('/').filter(Boolean).pop(),
value: String(param.Value ?? ''),
version: Number(param.Version || 0),
}))
.filter((param) => param.key)
.sort((a, b) => a.key.localeCompare(b.key))
if (records.length === 0) {
console.error(`sst-env-sync: no parameters found under ${prefix} in ${REGION}; seed SSM before running SST with BOXLITE_ENV_SOURCE=ssm`)
process.exit(1)
}
writeAtomically(outFile, `${records.map((param) => dotenvLine(param.key, param.value)).join('\n')}\n`)
const records = params
.map((param) => ({
key: String(param.Name || '').split('/').filter(Boolean).pop(),
value: String(param.Value ?? ''),
version: Number(param.Version || 0),
}))
.filter((param) => param.key)
.sort((a, b) => a.key.localeCompare(b.key))
const invalid = records.filter((param) => !/^[A-Za-z_][A-Za-z0-9_]*$/.test(param.key))
if (invalid.length) {
console.error(`sst-env-sync: invalid dotenv key(s): ${invalid.map((p) => p.key).join(', ')}`)
process.exit(1)
}
const seen = new Set()
const duplicates = records.filter((param) => {
if (seen.has(param.key)) return true
seen.add(param.key)
return false
})
if (duplicates.length) {
console.error(`sst-env-sync: duplicate dotenv key(s): ${duplicates.map((p) => p.key).join(', ')}`)
process.exit(1)
}
if (records.length === 0) {
console.error(`sst-env-sync: no parameters found under ${prefix} in ${REGION}; seed SSM before running SST with BOXLITE_ENV_SOURCE=ssm`)
process.exit(1)
}
writeAtomically(outFile, `${records.map((param) => dotenvLine(param.key, param.value)).join('\n')}\n`)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/infra/scripts/sst-with-cloudflare.mjs` around lines 163 - 177, The
SSM-to-dotenv mapping in records currently derives keys from only the last path
segment, which can create duplicate or invalid .env entries. Update the records
processing in sst-with-cloudflare.mjs to validate each derived key before
writeAtomically runs: reject empty or malformed dotenv keys and fail fast if two
params produce the same key. Use the existing records pipeline and dotenvLine
call site to add this check right after mapping/filtering, before sorting and
writing.

Comment thread apps/infra/sst.config.ts
Comment on lines +31 to +32
const ACCOUNT_ID = '064212132677'
const IAM_ROLE_BOUNDARY_ARN = `arn:aws:iam::${ACCOUNT_ID}:policy/boxlite-role-boundary`

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '--- apps/infra/sst.config.ts (outline) ---'
ast-grep outline apps/infra/sst.config.ts --view expanded || true

echo
echo '--- relevant file slices ---'
sed -n '1,180p' apps/infra/sst.config.ts

echo
echo '--- search for permissionsBoundary and account constants ---'
rg -n "permissionsBoundary|ACCOUNT_ID|IAM_ROLE_BOUNDARY_ARN|aws.getCallerIdentityOutput|dotenv\.config|run\(" apps/infra -S

echo
echo '--- package versions ---'
sed -n '1,220p' apps/infra/package.json

Repository: boxlite-ai/boxlite

Length of output: 1959


🏁 Script executed:

#!/bin/bash
set -euo pipefail
sed -n '1,180p' apps/infra/sst.config.ts

Repository: boxlite-ai/boxlite

Length of output: 1959


🌐 Web query:

Pulumi aws.iam.RoleArgs permissionsBoundary Input<string> @pulumi/aws version

💡 Result:

In the Pulumi AWS provider, the permissionsBoundary property within aws.iam.RoleArgs is defined as a Pulumi Input [1][2]. This means it accepts a string, a promise that resolves to a string, or an Output [1]. The permissionsBoundary property expects the Amazon Resource Name (ARN) of the IAM managed policy that you wish to set as the permissions boundary for the role [3][4][5]. Technical details: - Type: pulumi.Input | undefined [1][2] - Purpose: Specifies the ARN of the managed policy used to define the maximum permissions that identity-based policies can grant to the role [6][5]. - Behavior: This aligns with the underlying AWS IAM API and CloudFormation resource, where the permissions boundary is specified by its ARN [6][5]. For more information on using permissions boundaries, refer to the official AWS documentation on permissions boundaries for IAM entities [7].

Citations:


Avoid hard-coding the permissions boundary account.
A fixed 064212132677 will point every role at the wrong boundary when this stack is deployed to another account. Make the ARN configurable or derive the account from the deployment context.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/infra/sst.config.ts` around lines 31 - 32, The permissions boundary ARN
is hard-coded to a single account, which will break deployments in other
accounts. Update the ACCOUNT_ID/IAM_ROLE_BOUNDARY_ARN logic in sst.config.ts to
derive the account from the deployment context or make it configurable via
environment/config, and ensure the IAM_ROLE_BOUNDARY_ARN used by the stack
always matches the target account.

Comment on lines +571 to +580
restart_rollback_target() {
local target="\$1"
local saved_guest_expected="\${GUEST_EXPECTED:-}"
local saved_runtime_suffix="\${RUNTIME_SUFFIX:-}"
local saved_runtime_cache_dir_name="\${RUNTIME_CACHE_DIR_NAME:-}"

GUEST_EXPECTED=""
RUNTIME_SUFFIX=""
RUNTIME_CACHE_DIR_NAME="v${RUNTIME_CACHE_VERSION}"
restart_with_target "\$target"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Restore the previous runtime override during rollback.

Line 579 points rollback at v${RUNTIME_CACHE_VERSION}, which is derived from the failed upgrade, not the prior target. If rolling back from v0.9.7 to v0.9.6, the old binary can be restarted with the new runtime cache override and fail rollback. Capture and restore the pre-upgrade drop-in, or persist the runtime cache name per release target and use that for rollback.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/deploy/runner-update-binary.sh` around lines 571 - 580, The rollback
path in restart_rollback_target is using the failed upgrade’s runtime cache
override instead of the previously active one. Update restart_rollback_target
and restart_with_target handling so rollback restores the saved pre-upgrade
values for GUEST_EXPECTED, RUNTIME_SUFFIX, and especially
RUNTIME_CACHE_DIR_NAME, rather than always forcing v${RUNTIME_CACHE_VERSION}.
Use the existing saved_* locals in restart_rollback_target or persist the
runtime cache name per release target, then pass that restored value into
restart_with_target so the old binary restarts with its original runtime
override.

Comment on lines +600 to 611
if curl -fL --show-error --silent \
--retry 3 --retry-delay 2 --retry-connrefused \
--connect-timeout "${RUNNER_DOWNLOAD_CONNECT_TIMEOUT_SECONDS}" \
--max-time "${RUNNER_CHECKSUM_DOWNLOAD_MAX_TIME_SECONDS}" \
"${DOWNLOAD_SHA_URL}" -o "\$WORK/runner.sha256"; then
EXPECTED=\$(awk '{print \$1}' "\$WORK/runner.sha256")
ACTUAL=\$(sha256sum "\$WORK/runner.tar.gz" | awk '{print \$1}')
[ "\$EXPECTED" = "\$ACTUAL" ] || { echo "FATAL: checksum mismatch (want \$EXPECTED got \$ACTUAL)" >&2; exit 1; }
echo "checksum verified (\$ACTUAL)"
else
echo "WARNING: no .sha256 published for v${VERSION}; installing without integrity verification" >&2
echo "WARNING: no checksum available for runner tarball; installing without integrity verification" >&2
fi

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Fail closed when the checksum cannot be downloaded.

Line 610 allows installing the runner tarball without integrity verification. Since this is a privileged binary rollout path, a missing or transiently unavailable .sha256 should abort instead of continuing.

Proposed fix
 else
-  echo "WARNING: no checksum available for runner tarball; installing without integrity verification" >&2
+  echo "FATAL: checksum unavailable for runner tarball; refusing unverified install" >&2
+  exit 1
 fi
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if curl -fL --show-error --silent \
--retry 3 --retry-delay 2 --retry-connrefused \
--connect-timeout "${RUNNER_DOWNLOAD_CONNECT_TIMEOUT_SECONDS}" \
--max-time "${RUNNER_CHECKSUM_DOWNLOAD_MAX_TIME_SECONDS}" \
"${DOWNLOAD_SHA_URL}" -o "\$WORK/runner.sha256"; then
EXPECTED=\$(awk '{print \$1}' "\$WORK/runner.sha256")
ACTUAL=\$(sha256sum "\$WORK/runner.tar.gz" | awk '{print \$1}')
[ "\$EXPECTED" = "\$ACTUAL" ] || { echo "FATAL: checksum mismatch (want \$EXPECTED got \$ACTUAL)" >&2; exit 1; }
echo "checksum verified (\$ACTUAL)"
else
echo "WARNING: no .sha256 published for v${VERSION}; installing without integrity verification" >&2
echo "WARNING: no checksum available for runner tarball; installing without integrity verification" >&2
fi
if curl -fL --show-error --silent \
--retry 3 --retry-delay 2 --retry-connrefused \
--connect-timeout "${RUNNER_DOWNLOAD_CONNECT_TIMEOUT_SECONDS}" \
--max-time "${RUNNER_CHECKSUM_DOWNLOAD_MAX_TIME_SECONDS}" \
"${DOWNLOAD_SHA_URL}" -o "\$WORK/runner.sha256"; then
EXPECTED=\$(awk '{print \$1}' "\$WORK/runner.sha256")
ACTUAL=\$(sha256sum "\$WORK/runner.tar.gz" | awk '{print \$1}')
[ "\$EXPECTED" = "\$ACTUAL" ] || { echo "FATAL: checksum mismatch (want \$EXPECTED got \$ACTUAL)" >&2; exit 1; }
echo "checksum verified (\$ACTUAL)"
else
echo "FATAL: checksum unavailable for runner tarball; refusing unverified install" >&2
exit 1
fi
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/deploy/runner-update-binary.sh` around lines 600 - 611, The runner
checksum fallback in the download/install flow currently continues when the
.sha256 file cannot be fetched, which leaves the privileged binary rollout
unverified. Update the checksum handling in runner-update-binary.sh so the
branch around the curl/awk/sha256sum logic fails closed: if DOWNLOAD_SHA_URL
cannot be downloaded, abort the install path with a fatal error instead of
printing the warning and proceeding. Keep the behavior centered around the
existing checksum verification block and its EXPECTED/ACTUAL comparison.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant