chore: CI performance improvements and fix CORS test mocks#2081
chore: CI performance improvements and fix CORS test mocks#2081nick-inkeep merged 6 commits intomainfrom
Conversation
…verhead - Upgrade ci job runner from ubuntu-latest to ubuntu-16gb for more resources - Remove OpenTelemetry NodeSDK initialization from test setup (was creating full auto-instrumentation per worker thread with no benefit in unit tests) - Reduce agents-api vitest maxThreads from 10 to 8 and minThreads from 4 to 2 to better match runner core count and reduce per-worker initialization cost Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove @opentelemetry/exporter-trace-otlp-proto and @opentelemetry/sdk-metrics from agents-api devDependencies since they were only used in the test setup OTel initialization that was removed in the previous commit. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Three targeted fixes to prevent catastrophic cache invalidation in CI: 1. Exclude test files from build task inputs - prevents test file changes in core packages from cascading build hash invalidation to all 11+ downstream packages. Uses officially documented $TURBO_DEFAULT$ with negation globs (turbo 1.12+). 2. Remove transit dependency from lint task - transit is a no-op coordination task (no package defines a transit script) but its hash changes on any file change, cascading to all downstream lint tasks. Lint only reads local source files and doesn't need dependency ordering. 3. Move TURBO_TOKEN/TURBO_TEAM to job-level env - ensures all turbo invocations (check, knip) use remote cache, not just pnpm check. Also adds timeout-minutes: 30 as a safety guardrail. Evidence: PR #2068 changed 2 test files but caused 36/45 cache misses. With these fixes, the same change would cause ~6 misses (only the directly affected packages). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Commit 37e72ed gated localhost CORS on env.ENVIRONMENT but did not update the test mocks, causing 4 CORS tests to fail in CI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This PR delivers a focused set of CI performance improvements and a necessary test fix. The changes are well-scoped and correctly implemented:
- ✅ Runner upgrade (
ubuntu-latest→ubuntu-16gb) provides more resources - ✅ Job-level TURBO env vars enable consistent remote caching across all steps
- ✅ Build input exclusions (
!**/*.test.*,!**/__tests__/**) prevent cache cascade invalidation - ✅ Removing the
transitdependency from lint eliminates spurious cache misses - ✅ OTel test infrastructure removal reduces per-worker initialization overhead
- ✅ CORS test mock fixes correctly address the regression from #2066
No new issues introduced by this PR.
🧹 While You're Here (1) 🧹
🧹 1) .github/workflows/ci.yml CI workflow missing explicit permissions block
Issue: The CI workflow does not declare an explicit permissions: block. While the workflow likely runs with default read permissions on PRs from forks, other workflows in the repo (release.yml, ci-maintenance.yml, stale.yml) follow the pattern of explicit permissions blocks.
Why: Adding explicit permissions is a security best practice that documents intent and prevents unintended privilege escalation if the workflow is later modified.
Fix: Add a permissions block near the top of the workflow:
permissions:
contents: read
actions: readRefs:
- release.yml:14 — existing pattern
- ci-maintenance.yml:24 — existing pattern
✅ APPROVE
Summary: Clean, well-scoped CI performance optimizations with a necessary test fix. The changes are correctly implemented and improve cache efficiency. The "While You're Here" item about adding explicit permissions is optional but would bring ci.yml in line with other workflows in the repo.
Discarded (7)
| Location | Issue | Reason Discarded |
|---|---|---|
.github/workflows/ci.yml |
Actions pinned to mutable tags instead of SHAs | Pre-existing issue, significantly expands scope, lower priority than permissions |
.github/workflows/ci.yml:20-24 |
TURBO env vars moved to job-level scope | Positive validation — no issue, correct implementation |
turbo.json |
Build inputs correctly exclude test files | Positive validation — no issue, well-implemented |
turbo.json:92-95 |
lint task dependency on transit removed | Positive validation — transit was a no-op coordination task |
agents-api/src/__tests__/setup.ts |
OTel test infrastructure removed correctly | Positive validation — no issue, correct optimization |
agents-api/vitest.config.ts |
Thread pool settings reduced | Positive validation with monitoring suggestion — not an issue |
agents-api/src/__tests__/utils/cors.test.ts |
CORS test mocks correctly updated | Positive validation — fix is correct |
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-devops |
8 | 0 | 0 | 1 | 0 | 0 | 7 |
| Total | 8 | 0 | 0 | 1 | 0 | 0 | 7 |
Matches the pattern used by release.yml, ci-maintenance.yml, and stale.yml. Documents intent and prevents unintended privilege escalation if the workflow is later modified. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Delta Review Scope
This is a re-review scoped to the 1 commit since the last automated review:
aa361e0bf—chore: add explicit permissions block to CI workflow
Changes Reviewed
The delta adds an explicit permissions block to .github/workflows/ci.yml:
permissions:
contents: read
actions: readThis directly addresses the "While You're Here" recommendation from the previous review, which suggested adding explicit permissions for security best practices.
Validation
✅ Correctly implemented — The permissions follow the principle of least privilege (read-only access)
✅ Consistent with repo patterns — Matches the structure used in release.yml, ci-maintenance.yml, stale.yml, and inkeep-content-writer.yml
✅ Well-placed — Positioned at workflow level (after on: triggers, before concurrency:)
No new issues introduced.
✅ APPROVE
Summary: The author addressed the previous review's "While You're Here" suggestion by adding an explicit permissions block. The implementation correctly applies least-privilege read-only permissions and follows existing repo conventions. Ship it! 🚀
Reviewers (0)
No sub-reviewers dispatched — the delta (4 lines adding a standard permissions block) was too minimal to warrant subagent review.
1 participant
Summary
ENVIRONMENTcheck toisOriginAllowed()but did not update the test mocks, causing 4 CORS tests to fail in CIRoot cause of CORS test failures
Commit
37e72eda4(#2066) gated localhost/127.0.0.1 CORS onenv.ENVIRONMENT === 'development' || env.ENVIRONMENT === 'test'. The existing test mocks in bothcors.test.tsfiles didn't includeENVIRONMENT, so it wasundefinedand the localhost check failed.Fix: Added
ENVIRONMENT: 'development'to all development-mode and fallback test mocks in:agents-api/src/__tests__/utils/cors.test.tsagents-api/src/__tests__/manage/utils/cors.test.tsTest plan
🤖 Generated with Claude Code