From 78004746bec22a01876ea083fd754d63f6bafe73 Mon Sep 17 00:00:00 2001 From: "Kai (Tam Nhu) Tran" <61256810+kaitranntt@users.noreply.github.com> Date: Sat, 16 May 2026 13:57:50 -0400 Subject: [PATCH] fix: address upstream reviewer findings + failing CI checks (#1261 loop 1) (#1271) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(ci): breaking-change-guard skips missing base files (CI-1) Guard each git-show call with git cat-file -e existence check before attempting to read docker/compose.yaml from the base branch. When the file doesn't exist on the base (new file in this PR), the script would crash with "fatal: path exists on disk but not in origin/dev". New files can't cause a contract regression, so we exit early with breaking=0. * style: prettier reformat unrelated drift (CI-2) Two files had minor formatting drift from earlier umbrella PRs: - src/cliproxy/quota/quota-manager.ts - src/management/checks/image-analysis-check.ts No logic changes — formatter-only pass to unblock CI format:check. * fix(test): update trusted-author gate count to 4 in ci-workflow test (CI-3) PR #1260 added a compose-parity job to ci.yml. That job runs on self-hosted runners using PR-provided checkout, so it legitimately requires the trusted-author guard (same as validate, build, test). The test expected 3 occurrences; the correct count is now 4: validate (matrix), build, test, compose-parity. * fix(ci): smoke-test passes compose path + image ref to network-contract (REV-1) network-contract.sh signature is: [image-ref] The smoke-test job was calling it as: bash tests/docker/network-contract.sh "${{ steps.image.outputs.ref }}" which placed the image ref in the compose-file position ($1), causing the script to try docker compose -f which fails. Corrected to: bash tests/docker/network-contract.sh docker/compose.yaml "${{ steps.image.outputs.ref }}" Also removes publish-dashboard from smoke-test.needs (REV-2): when publish-dashboard is SKIPPED on prerelease events, GitHub Actions propagates the skip to downstream jobs, so smoke-test and promote-mutable-tags were silently skipped on every rc.N publish. smoke-test only verifies the integrated image; it has no dependency on the legacy dashboard image job. * docs(docker): annotate /root/.ccs path in compose volume (REV-3 clarification) The reviewer raised a concern that the compose volume mounts /root/.ccs but the entrypoint might default to /home/node/.ccs. This is a false positive: the integrated image uses entrypoint-integrated.sh (not entrypoint.sh), which runs under supervisord with user=root and explicitly mkdir -p /root/.ccs. HOME is /root inside the container. The volume mount at /root/.ccs is correct. Added an inline comment documenting the reasoning so future reviewers do not confuse entrypoint.sh (legacy dashboard image) with entrypoint-integrated.sh (integrated image). * fix(ci): gate docs-parity pull_request job to trusted authors docs-parity.yml runs on a self-hosted runner and checks out PR code. The self-hosted-runner-policy test requires any such workflow to include the trusted-author guard. The workflow was missing the guard, causing bun test:fast to fail with 1 failure. Allow push events (no author check needed — push is to own branch) and trusted-contributor PRs only. --- .github/workflows/breaking-change-guard.yml | 9 ++++++++- .github/workflows/docker-release.yml | 12 ++++++++++-- .github/workflows/docs-parity.yml | 3 +++ docker/compose.yaml | 5 +++++ src/cliproxy/quota/quota-manager.ts | 5 +++-- src/management/checks/image-analysis-check.ts | 5 +++-- tests/unit/scripts/github/ci-workflow.test.ts | 3 ++- 7 files changed, 34 insertions(+), 8 deletions(-) diff --git a/.github/workflows/breaking-change-guard.yml b/.github/workflows/breaking-change-guard.yml index 71420bda..6c3984fc 100644 --- a/.github/workflows/breaking-change-guard.yml +++ b/.github/workflows/breaking-change-guard.yml @@ -41,7 +41,14 @@ jobs: set -euo pipefail BASE="origin/${{ github.base_ref }}" - DIFF=$(git diff "${BASE}"...HEAD -- docker/compose.yaml) + + # Guard: if docker/compose.yaml does not exist in the base branch, this + # is a new file — no contract existed before, so no regression is possible. + if ! git cat-file -e "${BASE}:docker/compose.yaml" 2>/dev/null; then + echo "[i] docker/compose.yaml is new in this PR (not present on ${BASE}) — skipping contract check" + echo "breaking=0" >> "$GITHUB_OUTPUT" + exit 0 + fi BREAKING=0 diff --git a/.github/workflows/docker-release.yml b/.github/workflows/docker-release.yml index 056e0bb0..5061e873 100644 --- a/.github/workflows/docker-release.yml +++ b/.github/workflows/docker-release.yml @@ -288,7 +288,11 @@ jobs: # --------------------------------------------------------------------------- smoke-test: name: Smoke test integrated image - needs: [publish-integrated, publish-dashboard] + # Depends only on publish-integrated, NOT publish-dashboard. + # publish-dashboard is skipped on prerelease (rc.N) events — if it were + # listed here, GitHub Actions would also skip smoke-test and + # promote-mutable-tags on every rc publish, breaking the rc soak flow. + needs: [publish-integrated] # Run on both rc and stable releases; skip if integrated publish was skipped if: ${{ needs.publish-integrated.outputs.publish == 'true' }} runs-on: [self-hosted, linux, x64, cliproxy] @@ -355,7 +359,11 @@ jobs: - name: Run network-contract test run: | - bash tests/docker/network-contract.sh "${{ steps.image.outputs.ref }}" + # network-contract.sh signature: [image-ref] + # Pass compose.yaml as $1 and the pinned image ref as $2 so the + # smoke test exercises the canonical compose file with the exact + # image that was just published, not whatever tag is in the file. + bash tests/docker/network-contract.sh docker/compose.yaml "${{ steps.image.outputs.ref }}" - name: Probe dashboard port 3000 run: | diff --git a/.github/workflows/docs-parity.yml b/.github/workflows/docs-parity.yml index f51a316d..b7c7c20e 100644 --- a/.github/workflows/docs-parity.yml +++ b/.github/workflows/docs-parity.yml @@ -19,6 +19,9 @@ on: jobs: quickstart-parity: name: Assert quickstart snippet matches in README.md and docker/README.md + if: >- + contains(fromJSON('["COLLABORATOR","MEMBER","OWNER"]'), github.event.pull_request.author_association) + || github.event_name == 'push' runs-on: [self-hosted, linux, x64, cliproxy] steps: diff --git a/docker/compose.yaml b/docker/compose.yaml index 4f653d16..e4616cba 100644 --- a/docker/compose.yaml +++ b/docker/compose.yaml @@ -20,6 +20,11 @@ services: - "3000:3000" - "8317:8317" volumes: + # /root/.ccs matches the HOME used inside the integrated image. + # entrypoint-integrated.sh runs as root (supervisord user=root) and + # explicitly mkdir -p /root/.ccs, so state always lands here. + # Note: docker/entrypoint.sh (legacy dashboard image) uses a different + # default — it does NOT apply to this integrated image. - ccs_home:/root/.ccs - ccs_logs:/var/log/ccs networks: diff --git a/src/cliproxy/quota/quota-manager.ts b/src/cliproxy/quota/quota-manager.ts index 39841eb6..4493aeb6 100644 --- a/src/cliproxy/quota/quota-manager.ts +++ b/src/cliproxy/quota/quota-manager.ts @@ -524,8 +524,9 @@ export async function preflightCheck(provider: CLIProxyProvider): Promise * Fix image analysis configuration issues */ export async function fixImageAnalysisConfig(): Promise { - const { updateConfig, loadOrCreateUnifiedConfig } = - await import('../../config/config-loader-facade'); + const { updateConfig, loadOrCreateUnifiedConfig } = await import( + '../../config/config-loader-facade' + ); const config = loadOrCreateUnifiedConfig(); let fixed = false; diff --git a/tests/unit/scripts/github/ci-workflow.test.ts b/tests/unit/scripts/github/ci-workflow.test.ts index ae546e35..a7aa8788 100644 --- a/tests/unit/scripts/github/ci-workflow.test.ts +++ b/tests/unit/scripts/github/ci-workflow.test.ts @@ -19,7 +19,8 @@ describe('pr ci workflow', () => { expect(workflow).toContain('name: CI'); expect(workflow).toContain('pull_request:'); expect(workflow).toContain('branches: [main, dev]'); - expect(workflow.split(trustedAuthorGate).length - 1).toBe(3); + // 4 jobs: validate (matrix), build, test, compose-parity — each gated + expect(workflow.split(trustedAuthorGate).length - 1).toBe(4); expect(workflow).toContain('group: ci-${{ github.ref }}'); expect(workflow).toContain('cancel-in-progress: true'); expect(workflow).toContain('fail-fast: false');