ci(workflows): route Linux jobs to self-hosted p330 - #4
Conversation
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
📝 WalkthroughWalkthroughMultiple GitHub Actions workflows switch jobs to a labeled self-hosted runner; ci.yml and npm-publish.yml add workflow-level env vars to disable npm audit/fund and skip Playwright browser downloads; a unit test was updated to restore retry config and assert provider DB state after retry recovery. ChangesSelf-Hosted Runner Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.github/workflows/ci.yml:
- Line 26: The CI workflow uses custom self-hosted runner labels (nomenak,
omniroute, p330) in the runs-on array which actionlint flags as unknown; fix
this by adding an actionlint configuration that declares these labels in the
allowlist for runners (include nomenak, omniroute, p330) so actionlint
recognizes them, and ensure the new config is committed to the repo so all
workflows referencing the runs-on labels pass actionlint validation.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9f286e82-b751-46c5-b0a4-ff64f9aca87f
📒 Files selected for processing (5)
.github/workflows/ci.yml.github/workflows/deploy-vps.yml.github/workflows/docker-publish.yml.github/workflows/electron-release.yml.github/workflows/npm-publish.yml
![capy-ai[bot]](https://avatars.githubusercontent.com/in/1915919?s=60&v=4){kind=small}
| lint: | ||
| name: Lint | ||
| runs-on: ubuntu-latest | ||
| runs-on: [self-hosted, Linux, X64, nomenak, omniroute, p330] |
There was a problem hiding this comment.
[🟠 High] [🔵 Bug]
The PR changes pull_request CI jobs from ephemeral GitHub-hosted runners to a persistent self-hosted runner, which lets contributor-controlled PR code execute on internal infrastructure and increases runner compromise/lateral-movement risk even when secrets are restricted. ```yaml
.github/workflows/ci.yml
jobs:
lint:
name: Lint
runs-on: [self-hosted, Linux, X64, nomenak, omniroute, p330]
``` This matters because this workflow is triggered by pull_request and now routes broad job coverage (lint/build/tests/security scans) to the same self-hosted target. Keep untrusted PR workloads on `ubuntu-latest`, or gate self-hosted execution to trusted contexts only (for example, trusted actors/events or `pull_request_target` with strict checkout safeguards).
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/chat-cooldown-aware-retry.test.ts (1)
31-39: ⚡ Quick winRestore
delayMsin teardown to avoid config leakage between tests.Line 32 sets
BaseExecutor.RETRY_CONFIG.delayMs = 0, but no restore is done inafterEach. Restoring it (likemaxAttempts) keeps test isolation robust.Suggested patch
const originalRetryMaxAttempts = BaseExecutor.RETRY_CONFIG.maxAttempts; +const originalRetryDelayMs = BaseExecutor.RETRY_CONFIG.delayMs; @@ test.afterEach(async () => { + BaseExecutor.RETRY_CONFIG.delayMs = originalRetryDelayMs; BaseExecutor.RETRY_CONFIG.maxAttempts = originalRetryMaxAttempts; await resetStorage(); });🤖 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 `@tests/unit/chat-cooldown-aware-retry.test.ts` around lines 31 - 39, The test setup modifies BaseExecutor.RETRY_CONFIG.delayMs to 0 in test.beforeEach but never restores it, causing config leakage; update the teardown (test.afterEach) to restore delayMs to its original value (similar to how maxAttempts is restored) by saving the original delayMs before changing it in beforeEach and assigning it back in afterEach, referring to BaseExecutor.RETRY_CONFIG, test.beforeEach/test.afterEach and resetStorage to ensure isolation.
🤖 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 `@tests/unit/chat-cooldown-aware-retry.test.ts`:
- Around line 130-135: The test dereferences updatedConnection without asserting
it exists; change the test around providersDb.getProviderConnections so it first
asserts the returned array has at least one element (or that updatedConnection
is defined) before using updatedConnection.id and
updatedConnection.rateLimitedUntil; update assertions near
providersDb.getProviderConnections / updatedConnection to include a clear
failure message if no connection is returned so the test fails with a
descriptive assertion instead of a TypeError.
---
Nitpick comments:
In `@tests/unit/chat-cooldown-aware-retry.test.ts`:
- Around line 31-39: The test setup modifies BaseExecutor.RETRY_CONFIG.delayMs
to 0 in test.beforeEach but never restores it, causing config leakage; update
the teardown (test.afterEach) to restore delayMs to its original value (similar
to how maxAttempts is restored) by saving the original delayMs before changing
it in beforeEach and assigning it back in afterEach, referring to
BaseExecutor.RETRY_CONFIG, test.beforeEach/test.afterEach and resetStorage to
ensure isolation.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7d01d4ca-6f28-470c-ba14-2b73f541fd61
📒 Files selected for processing (1)
tests/unit/chat-cooldown-aware-retry.test.ts
| const [updatedConnection] = await providersDb.getProviderConnections({ provider: "openai" }); | ||
|
|
||
| assert.equal(response.status, 200); | ||
| assert.equal(fetchCalls, 4); | ||
| assert.equal(fetchCalls, 2); | ||
| assert.equal(updatedConnection.id, connection.id); | ||
| assert.equal(updatedConnection.rateLimitedUntil, undefined); |
There was a problem hiding this comment.
Assert connection existence before dereferencing for clearer failures.
Line 134 assumes updatedConnection exists. If Line 130 returns an empty array, this throws a TypeError instead of a precise test assertion.
Suggested patch
const [updatedConnection] = await providersDb.getProviderConnections({ provider: "openai" });
assert.equal(response.status, 200);
assert.equal(fetchCalls, 2);
+assert.ok(updatedConnection, "expected an updated openai provider connection");
assert.equal(updatedConnection.id, connection.id);
assert.equal(updatedConnection.rateLimitedUntil, undefined);📝 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.
| const [updatedConnection] = await providersDb.getProviderConnections({ provider: "openai" }); | |
| assert.equal(response.status, 200); | |
| assert.equal(fetchCalls, 4); | |
| assert.equal(fetchCalls, 2); | |
| assert.equal(updatedConnection.id, connection.id); | |
| assert.equal(updatedConnection.rateLimitedUntil, undefined); | |
| const [updatedConnection] = await providersDb.getProviderConnections({ provider: "openai" }); | |
| assert.equal(response.status, 200); | |
| assert.equal(fetchCalls, 2); | |
| assert.ok(updatedConnection, "expected an updated openai provider connection"); | |
| assert.equal(updatedConnection.id, connection.id); | |
| assert.equal(updatedConnection.rateLimitedUntil, undefined); |
🤖 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 `@tests/unit/chat-cooldown-aware-retry.test.ts` around lines 130 - 135, The
test dereferences updatedConnection without asserting it exists; change the test
around providersDb.getProviderConnections so it first asserts the returned array
has at least one element (or that updatedConnection is defined) before using
updatedConnection.id and updatedConnection.rateLimitedUntil; update assertions
near providersDb.getProviderConnections / updatedConnection to include a clear
failure message if no connection is returned so the test fails with a
descriptive assertion instead of a TypeError.
1 participant
This PR routes Linux CI/CD jobs to the deterministic p330 self-hosted runner and adds low-risk performance defaults to reduce CI/CD waste.
.github/workflows/ci.yml: Migrate all Linux jobs (lint, build, test-unit, test-coverage, test-e2e, test-integration, test-security, sonarqube, ci-summary, i18n-matrix, i18n, pr-test-policy, advanced-security, package-artifact, coverage-pr-comment) from
ubuntu-latestto[self-hosted, Linux, X64, nomenak, omniroute, p330]. Add workflow-levelNPM_CONFIG_AUDIT,NPM_CONFIG_FUND, andPLAYWRIGHT_SKIP_BROWSER_DOWNLOADdefaults. E2E job retains explicitnpx playwright install --with-deps chromium..github/workflows/docker-publish.yml: Pin runner from
[self-hosted, nomenak]to[self-hosted, Linux, X64, nomenak, omniroute, p330]for deterministic multi-arch builds..github/workflows/deploy-vps.yml: Move deploy job to p330 runner; triggers and SSH commands unchanged.
.github/workflows/npm-publish.yml: Move publish job to p330 runner; add npm/Playwright env defaults; preserve Node 22.22.2 and publish logic.
.github/workflows/electron-release.yml: Move validate and release jobs to p330 runner. Linux Electron matrix entry uses p330; macOS (intel/ARM64) and Windows entries remain on GitHub-hosted runners. Add workflow-level npm/Playwright env defaults.
Summary by CodeRabbit
Chores
Tests