From 6dc84bad7d227dbdc992672a8520db470b37fbd6 Mon Sep 17 00:00:00 2001 From: "andrii.dudar" Date: Wed, 19 Aug 2026 09:49:55 +0200 Subject: [PATCH 1/5] [OPIK-7793] [FE] ci: run the frontend checks with the private ai-spend plugin staged MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The comet frontend image compiles comet-ml/opik-plugin-ai-spend, checked out at build time into src/plugins/ai-spend. Nothing on a pull request compiles it — the image build runs on push to main, on release, and on PRs labelled test-environment — so a breaking change to a shared surface the plugin imports passes every PR check and only fails after merge. It has cost a red main (a constant moved out of CodeHighlighter) and, this week, two sequential release runs (OPIK-7793). Stage the plugin the way the image stages it and run typecheck, eslint and dependency-cruiser against the result. All three run even when an earlier one fails, so a PR sees every problem in one go; eslint is scoped to the plugin sources since core is already linted by the code quality workflow, and deps:validate is not scoped, since it is the whole-graph analysis that is how the plugin's own import cycle was found in the first place (fixed upstream in opik-plugin-ai-spend#69). Reviewed by baz-reviewer; findings and how each was resolved: - Trigger paths include build_and_push_docker.yaml, so a change to how the real image stages this same plugin re-runs the parity check. - persist-credentials: false on both checkouts. - Token availability is checked before ref parsing: a fork event (or a malformed ai-spend-plugin-ref in one) can no longer fail the job, since nothing downstream would use the ref anyway. - Staging validates manifest.ts specifically (name: "ai-spend"), using -type f so a directory literally named *.tsx can't inflate the count. PluginsStore loads plugins by that manifest; a layout change that drops or misnames it would otherwise leave production silently without the plugin's routes while this still passed on an unrelated file count. - A same-repo PR editing this file to read ${{ secrets.OPIK_PLUGIN_AI_SPEND_TOKEN }} directly was raised as high severity. Investigated rather than architecturally worked around: a workflow_run split (privileged half unreachable to a PR's own edits) was built, then dropped — it only closes editing this specific file, not adding a brand-new one with the same step, which any same-repo PR can already do to any secret here (see typescript_sdk_e2e_tests.yml, which already exposes OPENAI_API_KEY/ANTHROPIC_API_KEY the same way). Also confirmed empirically: this repo requires maintainer approval before any workflow runs for a first-time/outside contributor (verified via actual action_required runs on a recent fork PR), and the plugin's compiled source already ships publicly in the comet image's source maps regardless of this check. Net: the real residual risk is smaller than it first looked and specific to same-repo write access, which is the trust level every other secret here already assumes — closing it for real means an Environment with required reviewers on the secret itself, a repo-settings change, not a workflow one. - The "unmerged plugin ref" and "eslint skips .tsx without --ext" findings are addressed by reply, not code: the former is the documented, intended sequencing (verify against a branch, then merge it first); the latter is empirically wrong for this repo — a planted .tsx violation under src/plugins/development was caught by `eslint --max-warnings=0` with no --ext, exit 1, matching this repo's own root lint script. Verified against opik-plugin-ai-spend main (a disposable worktree, to avoid touching an unrelated in-progress checkout): typecheck, eslint, and deps:validate all exit 0 with the plugin staged, and clean with no plugin present and with it symlinked (the dev-runner layout). zizmor --pedantic: clean. --- .../frontend_private_plugin_checks.yml | 168 ++++++++++++++++++ 1 file changed, 168 insertions(+) create mode 100644 .github/workflows/frontend_private_plugin_checks.yml diff --git a/.github/workflows/frontend_private_plugin_checks.yml b/.github/workflows/frontend_private_plugin_checks.yml new file mode 100644 index 00000000000..9e738979316 --- /dev/null +++ b/.github/workflows/frontend_private_plugin_checks.yml @@ -0,0 +1,168 @@ +name: Frontend Private Plugin Checks +run-name: "Frontend Private Plugin Checks ${{ github.ref_name }} by @${{ github.actor }}" + +# The comet frontend image compiles comet-ml/opik-plugin-ai-spend, checked out at +# build time into src/plugins/ai-spend. Nothing else here compiles it, so a +# breaking change to a shared surface it imports passes every check on the PR +# that makes it and only fails later, in the image build. This runs the frontend +# checks with the plugin staged the same way the image stages it. +# +# For a change that intentionally breaks the plugin, name the plugin branch that +# adapts to it in the PR body, then merge the plugin PR first: +# +# ai-spend-plugin-ref: someone/my-branch +# +# Same-repo PRs get the token; that is the standard model this repo already +# uses for other secrets (e.g. typescript_sdk_e2e_tests.yml), and this repo +# additionally requires maintainer approval before any workflow runs for a +# first-time/outside contributor. Fork PRs get no token at all, from GitHub +# itself, regardless of what any workflow file says. + +permissions: + contents: read + +on: + pull_request: + paths: + - "apps/opik-frontend/**" + # A change to this check, or to the workflow that controls how the real + # image stages this same plugin, should re-run it. + - ".github/workflows/frontend_private_plugin_checks.yml" + - ".github/workflows/build_and_push_docker.yaml" + workflow_dispatch: + inputs: + ai_spend_plugin_ref: + type: string + required: false + description: ai-spend plugin ref + default: "main" + +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} + +jobs: + checks: + name: Checks with ai-spend plugin + runs-on: ubuntu-latest + timeout-minutes: 20 + steps: + - name: Checkout code + uses: actions/checkout@v7 + with: + fetch-depth: 1 + persist-credentials: false + + # Token availability checked first: without one, the job exits green with + # a notice, before ref parsing -- a malformed ai-spend-plugin-ref must + # never fail a fork event, since nothing downstream would use it anyway. + - name: Resolve plugin ref and token availability + id: resolve + env: + HAS_TOKEN: ${{ secrets.OPIK_PLUGIN_AI_SPEND_TOKEN != '' }} + DISPATCH_REF: ${{ inputs.ai_spend_plugin_ref }} + PR_BODY: ${{ github.event.pull_request.body }} + run: | + set -euo pipefail + echo "has_token=${HAS_TOKEN}" >> "$GITHUB_OUTPUT" + + if [ "${HAS_TOKEN}" != "true" ]; then + echo "::notice title=ai-spend plugin not checked::No access to the private plugin repository from this event. The plugin was not checked." + exit 0 + fi + + # Fenced blocks are skipped: a PR that documents this syntax in an + # example must not be taken as using it. + ref="${DISPATCH_REF:-}" + if [ -z "${ref}" ]; then + ref="$(printf '%s' "${PR_BODY:-}" \ + | awk '/^[[:space:]]*```/ { fenced = !fenced; next } !fenced' \ + | grep -iEm1 '^[[:space:]]*ai-spend-plugin-ref:[[:space:]]*[^[:space:]]+' \ + | sed -E 's/^[^:]*:[[:space:]]*//' \ + | tr -d '[:space:]' || true)" + fi + ref="${ref:-main}" + + if ! printf '%s' "${ref}" | grep -qE '^[A-Za-z0-9._/-]+$'; then + echo "::error title=Invalid ai-spend-plugin-ref::'${ref}' is not a valid git ref." + exit 1 + fi + + echo "ref=${ref}" >> "$GITHUB_OUTPUT" + echo "Plugin ref: ${ref}" + + - name: Checkout ai-spend plugin (private) + if: steps.resolve.outputs.has_token == 'true' + uses: actions/checkout@v7 + with: + repository: comet-ml/opik-plugin-ai-spend + ref: ${{ steps.resolve.outputs.ref }} + token: ${{ secrets.OPIK_PLUGIN_AI_SPEND_TOKEN }} + path: .ai-spend-plugin + fetch-depth: 1 + persist-credentials: false + + # Same staging as the image build. Asserted non-empty, and specifically + # checked for the manifest PluginsStore actually loads by name -- a layout + # change that drops or misnames manifest.ts would otherwise leave + # production silently without the plugin's routes while this still passed + # on an unrelated .ts file count. + - name: Stage ai-spend plugin into frontend src + if: steps.resolve.outputs.has_token == 'true' + run: | + set -euo pipefail + mkdir -p apps/opik-frontend/src/plugins/ai-spend + cp -R .ai-spend-plugin/src/. apps/opik-frontend/src/plugins/ai-spend/ + rm -rf .ai-spend-plugin + + count="$(find apps/opik-frontend/src/plugins/ai-spend -type f \( -name '*.ts' -o -name '*.tsx' \) | wc -l | tr -d ' ')" + if [ "${count}" -eq 0 ]; then + echo "::error title=Plugin staging produced nothing::Copied 0 TypeScript files. The plugin's src layout likely changed." + exit 1 + fi + + manifest=apps/opik-frontend/src/plugins/ai-spend/manifest.ts + if [ ! -f "${manifest}" ] || ! grep -qE 'name:[[:space:]]*"ai-spend"' "${manifest}"; then + echo "::error title=Plugin manifest missing or misnamed::PluginsStore loads plugins by the name declared in plugins/*/manifest.ts. ${manifest} is missing, or no longer declares name: \"ai-spend\" -- production would silently drop the plugin's routes." + exit 1 + fi + + echo "Staged ${count} TypeScript files from the plugin; manifest present and named correctly." + + - name: Set up Node.js + if: steps.resolve.outputs.has_token == 'true' + uses: actions/setup-node@v7 + with: + node-version: "20" + + - name: Install dependencies + if: steps.resolve.outputs.has_token == 'true' + run: npm ci + working-directory: apps/opik-frontend + + # All three run even if an earlier one fails, so a PR sees every problem in + # one go. eslint is scoped to the plugin: core files are already linted by + # the code quality workflow. + - name: Typecheck, lint and validate dependencies + if: steps.resolve.outputs.has_token == 'true' + working-directory: apps/opik-frontend + run: | + set -uo pipefail + failed=0 + + echo "::group::typecheck" + npm run typecheck || failed=1 + echo "::endgroup::" + + echo "::group::eslint (plugin sources)" + npx eslint src/plugins/ai-spend --max-warnings=0 || failed=1 + echo "::endgroup::" + + echo "::group::dependency-cruiser" + npm run deps:validate || failed=1 + echo "::endgroup::" + + if [ "${failed}" -ne 0 ]; then + echo "::error title=Frontend checks fail with the ai-spend plugin staged::Reproduce locally with a sibling opik-plugin-ai-spend checkout: bash scripts/dev-runner.sh --lint-fe. Keep the shared surface backward compatible, or land the matching plugin change first and add 'ai-spend-plugin-ref: ' to this PR body." + exit 1 + fi From 825c40d7c206d5dc48a34f5a6c32e93bf7d8e728 Mon Sep 17 00:00:00 2001 From: "andrii.dudar" Date: Wed, 19 Aug 2026 10:16:53 +0200 Subject: [PATCH 2/5] ci(frontend): give the exact reproduction commands in the failure annotation bash scripts/dev-runner.sh --lint-fe is not equivalent to what this check runs: it lints the whole src tree with --fix (not scoped to the plugin, mutating), plus stylelint, none of which this check does. A developer following it could see unrelated noise, or have a fixable issue silently rewritten without realizing CI's non-mutating, plugin-scoped eslint would still have failed on the committed version. Spell out the exact commands instead: symlink or copy the plugin's src/ into place, then run the same typecheck/eslint/deps:validate this step runs. --- .github/workflows/frontend_private_plugin_checks.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/frontend_private_plugin_checks.yml b/.github/workflows/frontend_private_plugin_checks.yml index 9e738979316..8050d3cd34d 100644 --- a/.github/workflows/frontend_private_plugin_checks.yml +++ b/.github/workflows/frontend_private_plugin_checks.yml @@ -163,6 +163,6 @@ jobs: echo "::endgroup::" if [ "${failed}" -ne 0 ]; then - echo "::error title=Frontend checks fail with the ai-spend plugin staged::Reproduce locally with a sibling opik-plugin-ai-spend checkout: bash scripts/dev-runner.sh --lint-fe. Keep the shared surface backward compatible, or land the matching plugin change first and add 'ai-spend-plugin-ref: ' to this PR body." + echo "::error title=Frontend checks fail with the ai-spend plugin staged::Reproduce locally: symlink or copy a sibling opik-plugin-ai-spend checkout's src/ into apps/opik-frontend/src/plugins/ai-spend, then from apps/opik-frontend run: npm run typecheck && npx eslint src/plugins/ai-spend --max-warnings=0 && npm run deps:validate. (bash scripts/dev-runner.sh --lint-fe is close but not equivalent -- it lints the whole src tree with --fix, plus stylelint, none of which this check runs.) Keep the shared surface backward compatible, or land the matching plugin change first and add 'ai-spend-plugin-ref: ' to this PR body." exit 1 fi From d498baa7a707b6e3de880724c33cf89d29fe4683 Mon Sep 17 00:00:00 2001 From: "andrii.dudar" Date: Thu, 20 Aug 2026 14:17:52 +0200 Subject: [PATCH 3/5] =?UTF-8?q?ci(frontend):=20address=20Liya's=20review?= =?UTF-8?q?=20=E2=80=94=20trigger=20types,=20staging=20order,=20robustness?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three requested changes: - pull_request.types now includes `edited`. Default types (opened, synchronize, reopened) don't cover an edited PR body, so the ai-spend-plugin-ref override documented in this file's own header silently didn't retrigger the check when added after opening -- discoverable only by pushing an empty commit. - Move Set up Node.js / npm ci ahead of the private plugin checkout and staging. PR-controlled postinstall scripts now run before the plugin source is on disk at all, rather than while it's staged. - Manifest grep accepts both quote styles (name: 'ai-spend' or "ai-spend"). It was asserting textually against a private repo's formatting; a prettier config change there to single quotes would otherwise turn every opik frontend PR red for reasons invisible to most reviewers. Also fixed, from the same review, all one-liners: - Stage step checks the plugin's src directory exists before cp -R. Verified empirically: cp -R against a missing source dies under set -e with a raw cp error, before ever reaching the "produced nothing" message meant for exactly this case. - Ref parsing takes the first whitespace-delimited token instead of stripping all whitespace: "main rebased" no longer concatenates to the invalid "mainrebased" (passes the allowlist, then fails opaquely at checkout). - A resolved ref other than "main" now emits a ::warning, so checking against an unmerged plugin branch -- or an unbalanced fence silently swallowing the rest of the body -- is visible on the PR, not just in the raw log. Verified against opik-plugin-ai-spend main (disposable worktree): typecheck, eslint, deps:validate all exit 0 with the plugin staged. Ref parser re-tested against all six prior cases plus "main rebased" -> resolves to "main". zizmor --pedantic: clean. --- .../frontend_private_plugin_checks.yml | 45 +++++++++++++------ 1 file changed, 31 insertions(+), 14 deletions(-) diff --git a/.github/workflows/frontend_private_plugin_checks.yml b/.github/workflows/frontend_private_plugin_checks.yml index 8050d3cd34d..25ecce326b6 100644 --- a/.github/workflows/frontend_private_plugin_checks.yml +++ b/.github/workflows/frontend_private_plugin_checks.yml @@ -23,6 +23,10 @@ permissions: on: pull_request: + # edited: the ai-spend-plugin-ref override lives in the PR body, so adding + # it after opening the PR must retrigger this -- default types + # (opened, synchronize, reopened) do not cover an edited description. + types: [opened, synchronize, reopened, edited] paths: - "apps/opik-frontend/**" # A change to this check, or to the workflow that controls how the real @@ -79,7 +83,7 @@ jobs: | awk '/^[[:space:]]*```/ { fenced = !fenced; next } !fenced' \ | grep -iEm1 '^[[:space:]]*ai-spend-plugin-ref:[[:space:]]*[^[:space:]]+' \ | sed -E 's/^[^:]*:[[:space:]]*//' \ - | tr -d '[:space:]' || true)" + | awk '{print $1}' || true)" fi ref="${ref:-main}" @@ -91,6 +95,23 @@ jobs: echo "ref=${ref}" >> "$GITHUB_OUTPUT" echo "Plugin ref: ${ref}" + if [ "${ref}" != "main" ]; then + echo "::warning title=Checking against a non-main plugin ref::This run verifies against '${ref}', not the plugin's main -- nothing enforces that branch is merged before this PR merges." + fi + + # Ahead of the plugin checkout: npm ci's PR-controlled postinstall runs + # with no private plugin source on disk yet. + - name: Set up Node.js + if: steps.resolve.outputs.has_token == 'true' + uses: actions/setup-node@v7 + with: + node-version: "20" + + - name: Install dependencies + if: steps.resolve.outputs.has_token == 'true' + run: npm ci + working-directory: apps/opik-frontend + - name: Checkout ai-spend plugin (private) if: steps.resolve.outputs.has_token == 'true' uses: actions/checkout@v7 @@ -106,11 +127,18 @@ jobs: # checked for the manifest PluginsStore actually loads by name -- a layout # change that drops or misnames manifest.ts would otherwise leave # production silently without the plugin's routes while this still passed - # on an unrelated .ts file count. + # on an unrelated .ts file count. The source directory is checked before + # copying: cp -R against a missing/renamed src fails under set -e with a + # raw cp error, before the friendlier "produced nothing" message below. - name: Stage ai-spend plugin into frontend src if: steps.resolve.outputs.has_token == 'true' run: | set -euo pipefail + if [ ! -d .ai-spend-plugin/src ]; then + echo "::error title=Plugin staging produced nothing::.ai-spend-plugin/src does not exist. The plugin's src layout likely changed." + exit 1 + fi + mkdir -p apps/opik-frontend/src/plugins/ai-spend cp -R .ai-spend-plugin/src/. apps/opik-frontend/src/plugins/ai-spend/ rm -rf .ai-spend-plugin @@ -122,24 +150,13 @@ jobs: fi manifest=apps/opik-frontend/src/plugins/ai-spend/manifest.ts - if [ ! -f "${manifest}" ] || ! grep -qE 'name:[[:space:]]*"ai-spend"' "${manifest}"; then + if [ ! -f "${manifest}" ] || ! grep -qE "name:[[:space:]]*['\"]ai-spend['\"]" "${manifest}"; then echo "::error title=Plugin manifest missing or misnamed::PluginsStore loads plugins by the name declared in plugins/*/manifest.ts. ${manifest} is missing, or no longer declares name: \"ai-spend\" -- production would silently drop the plugin's routes." exit 1 fi echo "Staged ${count} TypeScript files from the plugin; manifest present and named correctly." - - name: Set up Node.js - if: steps.resolve.outputs.has_token == 'true' - uses: actions/setup-node@v7 - with: - node-version: "20" - - - name: Install dependencies - if: steps.resolve.outputs.has_token == 'true' - run: npm ci - working-directory: apps/opik-frontend - # All three run even if an earlier one fails, so a PR sees every problem in # one go. eslint is scoped to the plugin: core files are already linted by # the code quality workflow. From e947b8e601b1817de707e500194baf077b3c80e4 Mon Sep 17 00:00:00 2001 From: "andrii.dudar" Date: Thu, 20 Aug 2026 15:57:25 +0200 Subject: [PATCH 4/5] ci(frontend): strip CR before parsing ai-spend-plugin-ref The previous commit's fix for "main rebased" concatenating into "mainrebased" (tr -d '[:space:]' -> awk '{print $1}') introduced a regression Liya found: awk's default field separator doesn't treat \r as one, so on a CRLF-authored PR body -- the normal case for the GitHub web UI, which is also the edited path just enabled in the same commit -- the CR stays attached to the captured ref and fails the validation regex on an otherwise valid value. Confirmed empirically: "ai-spend-plugin-ref: main\r\n" captured as "main\r" (5 bytes), rejected; the annotation then renders as "'main' is not a valid git ref", contradicting itself. Strip \r first, before the fence-toggle and the grep, so it protects both. Verified against all seven cases from the review: CRLF main, CRLF branch, "main rebased", the fenced doc example, no override, the injection attempt, and the differently-cased/whitespaced key -- all match. Also re-verified the full typecheck/eslint/deps:validate stack against opik-plugin-ai-spend main (disposable worktree): exit 0. zizmor --pedantic clean. --- .github/workflows/frontend_private_plugin_checks.yml | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/.github/workflows/frontend_private_plugin_checks.yml b/.github/workflows/frontend_private_plugin_checks.yml index 25ecce326b6..4a0f4b1e390 100644 --- a/.github/workflows/frontend_private_plugin_checks.yml +++ b/.github/workflows/frontend_private_plugin_checks.yml @@ -76,10 +76,15 @@ jobs: fi # Fenced blocks are skipped: a PR that documents this syntax in an - # example must not be taken as using it. + # example must not be taken as using it. CR stripped first: a body + # edited in the GitHub web UI is CRLF, and awk's default field + # separator does not treat \r as one, so it would otherwise stay + # attached to the captured ref and fail the validation below on an + # otherwise valid value. ref="${DISPATCH_REF:-}" if [ -z "${ref}" ]; then ref="$(printf '%s' "${PR_BODY:-}" \ + | tr -d '\r' \ | awk '/^[[:space:]]*```/ { fenced = !fenced; next } !fenced' \ | grep -iEm1 '^[[:space:]]*ai-spend-plugin-ref:[[:space:]]*[^[:space:]]+' \ | sed -E 's/^[^:]*:[[:space:]]*//' \ From 3d4c601729a0f804e16b327c9026b70b17bc88a6 Mon Sep 17 00:00:00 2001 From: "andrii.dudar" Date: Thu, 20 Aug 2026 16:00:54 +0200 Subject: [PATCH 5/5] ci(frontend): distinguish the two 'staging produced nothing' error titles Missing src/ and zero .ts files were both titled 'Plugin staging produced nothing'. Bodies distinguish them; titles now do too. --- .github/workflows/frontend_private_plugin_checks.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/frontend_private_plugin_checks.yml b/.github/workflows/frontend_private_plugin_checks.yml index 4a0f4b1e390..521350994b0 100644 --- a/.github/workflows/frontend_private_plugin_checks.yml +++ b/.github/workflows/frontend_private_plugin_checks.yml @@ -140,7 +140,7 @@ jobs: run: | set -euo pipefail if [ ! -d .ai-spend-plugin/src ]; then - echo "::error title=Plugin staging produced nothing::.ai-spend-plugin/src does not exist. The plugin's src layout likely changed." + echo "::error title=Plugin src directory missing::.ai-spend-plugin/src does not exist. The plugin's src layout likely changed." exit 1 fi