ci: fall back to GitHub-hosted runners in forks - #13364
Conversation
Blacksmith runners are scoped to the Kilo-Org organisation, so every job pinned to a blacksmith-* label queues indefinitely in a fork and is eventually cancelled without running. Core test CI is therefore dead in forks: contributors cannot validate a change before opening a PR. Select the runner from github.repository instead of hardcoding it, so the canonical repo keeps its Blacksmith runners byte-for-byte and forks fall back to the GitHub-hosted equivalent. The test.yml unit matrix bakes hosts into JSON, so it routes through emit_settings, which substitutes the two labels from LINUX_HOST/WINDOWS_HOST. pull_request runs execute in the base repo, so PRs sent to Kilo-Org are unaffected; only pushes and PRs inside a fork change behaviour. Follow-up to (11210), where these guards were trimmed on the review note "core test infrastructure (if this fails on forks we should look into that instead)". Agent-Signature: claude-opus-5-high on behalf of matt wilkie
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (5 files)
Previous Review Summaries (2 snapshots, latest commit f66d5a5)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit f66d5a5)Status: No Issues Found | Recommendation: Merge Files Reviewed (5 files)
Previous review (commit 2ce950c)Status: No Issues Found | Recommendation: Merge Files Reviewed (6 files)
Reviewed by grok-4.6 · Input: 114.3K · Output: 6.5K · Cached: 272.1K Review guidance: REVIEW.md from base branch |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ce950c600
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # kilocode_change start - forks have no Blacksmith runners, so a matrix pinned to | ||
| # them queues forever and is cancelled without ever running. Emit GitHub-hosted | ||
| # equivalents outside the canonical repo. | ||
| LINUX_HOST: ${{ github.repository == 'Kilo-Org/kilocode' && 'blacksmith-4vcpu-ubuntu-2404' || 'ubuntu-latest' }} |
There was a problem hiding this comment.
In a fork with general changes, this routes the package-bearing Linux shard to GitHub-hosted Ubuntu, but the repository records that GitHub-hosted Ubuntu's AppArmor policy prevents the live Bubblewrap user-namespace bootstrap (.github/workflows/publish.yml:199-204). The shard still exports the custom helper at test.yml:165-168 and runs the non-CLI suite at test.yml:199-201, which includes packages/core/test/kilocode/linux-sandbox.test.ts:160-163 and unconditionally requires backendSupport().available; consequently the newly enabled fork shard and its required check fail even for valid changes. Adjust the GitHub-hosted runner's namespace policy or otherwise accommodate unavailable sandbox support before running these tests.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and fixed by removing test.yml from this PR (f66d5a5).
I checked the three links in the chain rather than taking the finding on trust, and they hold:
.github/workflows/publish.yml:199-204already documents that GitHub-hosted Ubuntu 24.04 setskernel.apparmor_restrict_unprivileged_userns=1, and deliberately keeps its own live probe non-fatal because of it.packages/kilo-sandbox/src/bubblewrap.ts:241-256resolvesbackendSupport().availableby really executing the helper with--unshare-user --disable-userns, which is precisely what that sysctl blocks.packages/core/test/kilocode/linux-sandbox.test.ts:161then assertsexpect(support.available, support.reason).toBe(true)unconditionally on Linux.
So the fallback would have converted a cancelled required check into a failing one for valid changes — worse than the status quo. test.yml is now reverted to its base state, and the PR keeps the fallback only on the five workflows that never touch the sandbox.
The obvious fix is a step in .github/actions/setup-linux-sandbox relaxing that sysctl when set (CI-only, no-op on Blacksmith images). I have left it out on purpose: weakening a kernel security control on CI runners should be an explicit maintainer decision, not a side effect of a runner-selection change.
claude-opus-5-high on behalf of matt wilkie
The Linux sandbox suite cannot pass on a GitHub-hosted runner. Ubuntu 24.04 ships kernel.apparmor_restrict_unprivileged_userns=1, which blocks the Bubblewrap user-namespace bootstrap that backendSupport() probes, and packages/core/test/kilocode/linux-sandbox.test.ts asserts support.available is true unconditionally on Linux. Routing test.yml to ubuntu-latest in forks would therefore turn a cancelled required check into a failing one. Revert test.yml to its base state and keep the fallback on the five workflows that do not exercise the sandbox. Making test.yml fork-capable needs a decision about relaxing that sysctl on CI runners, which is Kilo-Org's call, not something to smuggle into a runner-selection change. Reported by the Codex review bot on PR 13364. Agent-Signature: claude-opus-5-high on behalf of matt wilkie
maphew
changed the title
|
Thanks for the contribution! merged. |
2 participants
Why
Blacksmith runners are scoped to the
Kilo-Orgorganisation. Any job pinned to ablacksmith-*label cannot be allocated a runner in a fork, so it sits queued and iseventually cancelled without ever executing.
The practical effect is that CI is dead in forks. In my own fork, every historical run of
typecheck,test-vscodeanddocs-buildhas the same conclusion:cancelled. Not oncea pass, not once a real failure. A contributor cannot validate a change on their own fork
before opening a PR here, and they get a wall of grey runs that look like their branch is
broken.
This is the follow-up @markijbema asked for in #11210:
I trimmed those guards at the time. This is the "look into that instead" half: rather
than disabling CI in forks, make it actually run there.
What
Select the runner from
github.repositoryinstead of hardcoding the label:11
runs-onlines acrosstypecheck.yml,test-vscode.yml,docs-build.yml,test-jetbrains.ymlandvisual-regression.yml. No job graph,if:condition,needs:,timeout or step is changed.
test.ymlis deliberately excludedI originally included it. The Codex review bot flagged that it cannot work, and it was
right:
kernel.apparmor_restrict_unprivileged_userns=1. This repo alreadydocuments that at
.github/workflows/publish.yml:199-204, and makes its own live probenon-fatal for exactly that reason.
packages/kilo-sandbox/src/bubblewrap.ts:241-256decidesbackendSupport().availableby actually running the helper with
--unshare-user --disable-userns, which thatsysctl blocks.
packages/core/test/kilocode/linux-sandbox.test.ts:161then assertsexpect(support.available, support.reason).toBe(true)unconditionally on Linux.So routing
test.ymltoubuntu-latestwould convert a cancelled required check into afailing one for valid changes. That is worse than the status quo, not better.
The fix would be one step in
.github/actions/setup-linux-sandboxrelaxing that sysctlwhen it is set — CI-only, a no-op on Blacksmith images. I have not included it here,
because relaxing a kernel security control on CI runners is your call to make explicitly,
not something to fold silently into a runner-selection PR. Happy to add it in this PR or a
follow-up if you want it; equally happy for the answer to be "no, the sandbox suite stays
Blacksmith-only".
Behaviour
Kilo-Org/kilocodeKilo-Org/kilocodeThe middle row is the obvious thing to worry about, so to be explicit:
pull_requestrunsexecute in the context of the base repository, so
github.repositoryisKilo-Org/kilocodefor a fork PR and Blacksmith is still selected. Contributor PRs to thisrepo are byte-for-byte unaffected. Verified against a live fork PR
(#13333), where
unit (linux, 1/2)passes on Blacksmith in 4m52s.Only pushes and PRs that stay inside a fork change behaviour.
Considered and rejected
Guarding these jobs off in forks with
if: github.repository == 'Kilo-Org/kilocode'— thatwas the original shape of #11210 and was correctly rejected. It removes
contributor CI rather than fixing it.
Driving the label from an org variable (
vars.CI_RUNNER_4VCPU) is tidier, but it failsopen: if the variable is ever unset or misspelled, this repo silently downgrades to
GitHub-hosted runners and nobody notices. The explicit repository check fails safe —
Kilo-Org/kilocodealways gets Blacksmith with no configuration required, so there isnothing for you to set up before merging.
Validation
yq eval '.' .github/workflows/*.yml— all 29 workflows parseactionlinton the five changed files — clean, exit 0bun run script/check-workflows.ts— ok (29 workflows)docs-buildneeds no secret in forks:instrumentation-client.tsguards onif (process.env.NEXT_PUBLIC_POSTHOG_KEY), so an empty value is fineupstream
Remaining caveat, stated plainly rather than left for you to find: I can show these
workflows will now run in a fork, not that they will pass.
ubuntu-latestis slowerand smaller than a 4vCPU Blacksmith box, so a fork may hit an existing timeout on the
heavier jobs. That is still strictly better than a cancelled queue — it is visible and
fixable, whereas today there is no signal at all — but it is why I would rather land this
without
test.ymland see how it behaves first.claude-opus-5-high on behalf of matt wilkie