From e7ce699dc25683b5a9a9b3fe345a8a87a823225b Mon Sep 17 00:00:00 2001 From: "Kai (Tam Nhu) Tran" <61256810+kaitranntt@users.noreply.github.com> Date: Sun, 17 May 2026 06:01:25 -0400 Subject: [PATCH] =?UTF-8?q?fix(ci):=20reviewer=20loop=206=20=E2=80=94=20RE?= =?UTF-8?q?V12=20compose=20image=20parsing,=20REV13=20fork=20bypass,=20REV?= =?UTF-8?q?14=20:full=20audit=20(#1278)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(ci): breaking-change-guard handles \${VAR:-default} compose image syntax (REV12) The sed 's/:.*//' pattern truncated at the first colon in the \${CCS_IMAGE:-ghcr.io/...} expression, yielding "\${CCS_IMAGE" as the image name instead of the actual registry path. Any change to the default image namespace was therefore undetectable. Introduce extract_image_name() that first strips the \${VAR:-default} wrapper with a sed -E expression, then strips only the trailing :tag suffix using a pattern that preserves internal colons (e.g. registry:5000/ owner/repo). Applied to both OLD_RAW and NEW_RAW extraction paths. * fix(ci): breaking-change-guard runs on ubuntu-latest to cover forked PRs (REV13) The trusted-author gate (COLLABORATOR|MEMBER|OWNER) caused forked-PR contributors to bypass the breaking-change check entirely. A forked contributor could rename services.ccs or change the image namespace without a feat!/fix! marker and the guard would never run. This workflow is a documented exception to the self-hosted-first policy: it performs ONLY pure YAML diff parsing (git show / awk / sed). No build, install, or arbitrary PR-branch scripts are executed. The checkout uses persist-credentials: false. There is no untrusted code execution, so ubuntu-latest is safe and necessary for universal fork coverage. Update self-hosted-runner-policy.test.ts to: - Introduce GITHUB_HOSTED_RUNNER_EXCEPTIONS registry with required justification comments for each entry - Skip exception workflows in the "keeps active workflows on local runners" and "gates pull-request workflows" assertions - Add a new "documented exceptions use github-hosted runners" test that verifies each exception entry actually uses a GitHub-hosted runner (prevents stale entries accumulating without cleanup) --- .github/workflows/breaking-change-guard.yml | 38 +++++++++++++++---- .../github/self-hosted-runner-policy.test.ts | 30 +++++++++++++++ 2 files changed, 61 insertions(+), 7 deletions(-) diff --git a/.github/workflows/breaking-change-guard.yml b/.github/workflows/breaking-change-guard.yml index c00ce7b6..876bfb1f 100644 --- a/.github/workflows/breaking-change-guard.yml +++ b/.github/workflows/breaking-change-guard.yml @@ -22,9 +22,17 @@ on: jobs: guard: name: Verify breaking changes are intentional - if: >- - contains(fromJSON('["COLLABORATOR","MEMBER","OWNER"]'), github.event.pull_request.author_association) - runs-on: [self-hosted, linux, x64, cliproxy] + # Exception to self-hosted-first policy (documented in CLAUDE.md "Self-Hosted Runner Policy"): + # This workflow MUST cover ALL PRs including forks — a forked contributor can rename + # services.ccs or change the image namespace without a breaking-change marker, which + # silently breaks sibling-container setups for every user. Gating on trusted-author + # association would let forked PRs bypass the check entirely. + # + # Safety justification: this workflow does ONLY pure YAML diff parsing (git show / git diff + # + shell + awk). It checks out the code with persist-credentials: false and runs no build, + # install, or arbitrary scripts from the PR branch. There is no untrusted code execution, + # so running on a GitHub-hosted runner is safe and required for universal coverage. + runs-on: ubuntu-latest steps: - name: Checkout PR branch @@ -54,10 +62,26 @@ jobs: BREAKING=0 # 1. Image name change (repo path, not tag — tags change every release) - OLD_IMAGE=$(git show "${BASE}:docker/compose.yaml" \ - | grep -m1 '^\s*image:' | sed 's/.*image:\s*//' | sed 's/:.*//' | tr -d ' ') - NEW_IMAGE=$(grep -m1 '^\s*image:' docker/compose.yaml \ - | sed 's/.*image:\s*//' | sed 's/:.*//' | tr -d ' ') + # + # compose.yaml uses ${CCS_IMAGE:-ghcr.io/kaitranntt/ccs:latest}. A naive + # sed 's/:.*//' truncates at the FIRST colon, yielding "${CCS_IMAGE" instead + # of the actual image name. We strip the ${VAR:-default} wrapper first, then + # strip only the trailing :tag suffix (preserving internal colons such as + # those in registry:port/owner/repo). + extract_image_name() { + # $1 = raw image line content (everything after "image: ") + local raw="$1" + # Strip ${VAR:-default} wrapper if present + raw=$(printf '%s' "$raw" | sed -E 's/^\$\{[A-Za-z_][A-Za-z0-9_]*:-//; s/\}$//') + # Strip only the trailing :tag — preserve internal colons (e.g. registry:5000/owner/repo) + printf '%s' "$raw" | sed 's|:[^:/]*$||' + } + OLD_RAW=$(git show "${BASE}:docker/compose.yaml" \ + | grep -m1 '^\s*image:' | sed 's/.*image:\s*//' | tr -d ' ') + NEW_RAW=$(grep -m1 '^\s*image:' docker/compose.yaml \ + | sed 's/.*image:\s*//' | tr -d ' ') + OLD_IMAGE=$(extract_image_name "$OLD_RAW") + NEW_IMAGE=$(extract_image_name "$NEW_RAW") if [[ "${OLD_IMAGE}" != "${NEW_IMAGE}" ]]; then echo "[!] BREAKING: image name changed: '${OLD_IMAGE}' -> '${NEW_IMAGE}'" BREAKING=1 diff --git a/tests/unit/scripts/github/self-hosted-runner-policy.test.ts b/tests/unit/scripts/github/self-hosted-runner-policy.test.ts index 6ca9313b..f9e18593 100644 --- a/tests/unit/scripts/github/self-hosted-runner-policy.test.ts +++ b/tests/unit/scripts/github/self-hosted-runner-policy.test.ts @@ -7,6 +7,17 @@ function workflowsDir() { return path.resolve(import.meta.dir, '../../../../.github/workflows'); } + // Documented exceptions to the self-hosted-first policy (see CLAUDE.md "Self-Hosted Runner Policy"). + // Each entry must include a justification comment explaining why GitHub-hosted runners + // are required for correctness (not just convenience). + const GITHUB_HOSTED_RUNNER_EXCEPTIONS: Record = { + // Pure YAML diff parser — no untrusted code execution. Must cover ALL PRs including + // forks to prevent forked contributors from bypassing the breaking-change check. + // Gating on trusted-author association would silently allow contract-breaking changes + // from forks. No build, install, or arbitrary PR-branch scripts are run here. + 'breaking-change-guard.yml': 'ubuntu-latest — fork-safe YAML diff check; no untrusted code execution', + }; + describe('self-hosted runner policy', () => { test('keeps active workflows on local runners', () => { const hostedRunnerLabels = [ @@ -23,6 +34,9 @@ describe('self-hosted runner policy', () => { expect(workflowFiles.length).toBeGreaterThan(0); for (const file of workflowFiles) { + // Skip files with documented, justified exceptions to the self-hosted-first policy + if (GITHUB_HOSTED_RUNNER_EXCEPTIONS[file]) continue; + const workflow = fs.readFileSync(path.join(workflowsDir(), file), 'utf8'); for (const label of hostedRunnerLabels) { @@ -35,6 +49,17 @@ describe('self-hosted runner policy', () => { } }); + test('documented exceptions use github-hosted runners for justified safety reasons', () => { + // Verify each documented exception actually uses a GitHub-hosted runner + // (prevents stale exception entries that no longer reflect the workflow) + for (const [file, reason] of Object.entries(GITHUB_HOSTED_RUNNER_EXCEPTIONS)) { + const workflow = fs.readFileSync(path.join(workflowsDir(), file), 'utf8'); + const hasGitHubHosted = ['ubuntu-latest', 'ubuntu-24.04', 'ubuntu-22.04', 'macos-latest', 'windows-latest'] + .some((label) => workflow.includes(`runs-on: ${label}`)); + expect(hasGitHubHosted, `${file} is in exceptions list (reason: ${reason}) but does not use a GitHub-hosted runner — remove the exception or restore the runner type`).toBe(true); + } + }); + test('gates pull-request self-hosted worker deploys to trusted authors', () => { const workflow = fs.readFileSync(path.join(workflowsDir(), 'deploy-ccs-worker.yml'), 'utf8'); @@ -60,6 +85,11 @@ describe('self-hosted runner policy', () => { ); } + // Documented exceptions run on GitHub-hosted runners for justified safety reasons + // (e.g. must cover forked PRs, no untrusted code execution). These workflows do not + // use self-hosted runners for their PR jobs, so the trusted-author gate does not apply. + if (GITHUB_HOSTED_RUNNER_EXCEPTIONS[file]) continue; + if ( workflow.includes('pull_request:') && workflow.includes('self-hosted') &&