Skip to content

fix(ci): isolate the dispatcher's KVM probe - #7598

Merged
moq-bot[bot] merged 8 commits into
mainfrom
quest/m0/ci/dispatch-test-kvm
Nov 2, 2026
Merged

moq-bot[bot] merged 8 commits into
mainfrom
quest/m0/ci/dispatch-test-kvm

Conversation

@kixelated

@kixelated kixelated commented Nov 2, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

The dispatcher's mocked CI runs still inspect the host's /dev/kvm. Hiding that device reproduces 20 failures across the 30 existing tests, although no VM is started.

Approach

Extract the unchanged existence probe into sh/kvm.sh and stub that fixed helper in the dispatcher fixture. Cover missing KVM in local and strict CI modes: fail the flake lane, name /dev/kvm, never invoke nix flake check, and finish independent modules. Existing capable-builder tests cover available KVM. Include the helper in the orchestration selector, cover helper-only edits, and remove the completed quest and its link.

Impact

  • No public API or wire changes. Linux x86_64 production checks still refuse missing /dev/kvm with the same diagnostic and exit status.

Alternatives

A configurable device path would add a production override solely for tests. A fixed probe helper keeps that control in the fixture.

Decisions

Used a fixed helper rather than an environment override; covered both local and strict CI refusal.

Checks

Gate on merge head eee0fed010 (main at da5c2afade): quest gate nix develop --ignore-environment --keep HOME --keep USER --keep PATH --command just ci origin/main, with the dev shell's PATH deduplicated as moq-dev/moq#7594 does (mbx's E2BIG came from nested shells duplicating PATH). 1553 s wall time; every lane passed except the two below, neither caused by this PR.

  • All 31 sh/dispatch-query.test.py tests pass. Before the fix, 20 failed with /dev/kvm hidden in a private Bubblewrap /dev mount; after it, all pass in the same namespace. The real helper returns 0 with /dev/kvm present and 1 with it hidden.
  • check flake failed: github-runner.drv stops at unshare: write failed /proc/self/uid_map: Operation not permitted. This host sets kernel.apparmor_restrict_unprivileged_userns=1 without the nix-repart-unshare profile from nix/runner/unshare.nix. Main fails identically here. That's by design: strict just ci requires the full flake check and never probes (moq-dev/moq#6823, doc/setup/dev.md). The non-strict probe is correct on this host: nix build --file nix/runner/userns-probe.nix exits 77 with MOQ_USERNS_UNAVAILABLE (the same uid_map refusal), so just check reports flake as skipped. An earlier report that the probe passes here was wrong.
  • test js failed 1 of 2689: a peer close drains the overtaken fin over moq-lite-02 at js/net/src/lite/connection.test.ts:394 (expected "frame 0", received undefined). Run alone on the same head, the file failed 2 of 57: the lite-03 and lite-04 cases. This PR changes nothing under js/ (git diff origin/main HEAD -- js is empty). The flake is planned as /quest/m0/test-flakes-2/js-close-drain.md in moq-dev/moq#7590. Not bypassed.
  • quest check passed.
  • Head bda683464c merges main bc2468e8be, which changes only quest/. On that head, quest check --base origin/main passes and all 31 dispatcher tests pass again.

Review

mechanical fix by quest land watch: merged main at 6944d3d2a8e8707032067be9651ec467ad78d036 with the quest merge driver (head 62c7d45)

mechanical fix by quest land watch: merged main at 04e9b36f89ad108fa1de5c84fbe6ba9d75ca2d28 with the quest merge driver (head 36309c5)

Claude Code, claude-opus-5-5, medium effort, round 2 reviewed ee7a55e22ae4b7aee095840397c90d0102ccb8aa against origin/main (21d6da9fc108b48373a961f34e43c3fcd38b0146): no findings. Since then the branch has had only conflict-free merges from main (eee0fed010, bda683464c), and the PR's patch is unchanged. The guide's review exemption covers head bda683464c40d93cd7f22b0f36343b63f07a3d1b.

  • Fixed: include sh/kvm.sh in the orchestration selector and cover helper-only edits.
  • Refuted: the suggested environment override could point at /dev/null and admit a production check without KVM. The fixed helper preserves the exact /dev/kvm predicate; the real helper was also checked with the device present and hidden. The reviewer accepted the response in round 2.

Follow-ups

  • None new. The JS close-drain flake is already planned in moq-dev/moq#7590. The uid_map refusal is host policy, which nix/runner/README.md documents.

(Written by GPT-6.1 Sol; updated by Claude Opus 5.5)

kixelated and others added 4 commits November 1, 2026 18:09
Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
@kixelated kixelated changed the title dispatch.sh's tests pass without KVM fix(ci): isolate the dispatcher's KVM probe Nov 2, 2026
@kixelated
kixelated marked this pull request as ready for review November 2, 2026 06:50
@coderabbitai

coderabbitai Bot commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: e97782ad-1813-4ea0-9eea-e0006c4749a5
Commits

Reviewing files that changed from the base of the PR and between 36309c5 and 62c7d45.

Files selected for processing: 2
  • quest/m0/ci/README.md
  • sh/dispatch-query.test.py
Files with no reviewable changes: 1
  • quest/m0/ci/README.md

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.

Walkthrough

The dispatcher now uses sh/kvm.sh to check for /dev/kvm before the Linux x86_64 flake check. If the probe fails, the flake check fails without being skipped, and gh test continues. Tests cover this behavior with strict CI enabled and disabled. The CI requirement entry and planning document for the work are removed.

Merge Risk: Minimal · up to 62c7d

The KVM probe is isolated, and no material merge risk remains.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title "fix(ci): isolate the dispatcher's KVM probe" matches the main change. The PR extracts the KVM existence probe into sh/kvm.sh and stubs it in the dispatcher test fixture, so the dispatcher's…
Description check Passed The description states the problem (mocked dispatcher CI runs inspect the host's /dev/kvm, causing 20 of 30 tests to fail when the device is hidden) and the approach (a fixed helper in sh/kvm.sh stubb…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title "fix(ci): isolate the dispatcher's KVM probe" matches the main change. The PR extracts the KVM existence probe into sh/kvm.sh and stubs it in the dispatcher test fixture, so the dispatcher's mocked CI runs no longer inspect the host's /dev/kvm. The title is concise, specific, and follows a conventional-commit style.

Full details: Description check

Explanation

The description states the problem (mocked dispatcher CI runs inspect the host's /dev/kvm, causing 20 of 30 tests to fail when the device is hidden) and the approach (a fixed helper in sh/kvm.sh stubbed in the fixture, with coverage for missing KVM in local and strict CI modes). It also covers impact, alternatives, and check results, all of which relate directly to the changeset.

Full details: Docstring Coverage

Explanation

Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 4 functions across 3 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches
Simplify code
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by review (OpenAI)

Reviewed commit: bda6834

No actionable bugs found. The fixed KVM probe helper is a small, appropriate test seam: it preserves the production /dev/kvm predicate without adding a runtime override. Helper-only edits select orchestration checks, and the new refusal test covers both local and strict CI modes while ensuring independent modules continue.

Verification: static review of the dispatcher, fixture setup, helper and changed tests. I did not run tests, Nix builds, or device-isolation checks; the PR's reported host-policy and JS-test failures were not independently reproduced.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

Reviewed the full diff of bda683464 against main (bc2468e8b): 5 files.

  • KVM probe extracted to sh/kvm.sh — sh/dispatch.sh calls bash sh/kvm.sh instead of inlining ! -e /dev/kvm, preserving the Linux x86_64 refusal and its /dev/kvm is missing diagnostic and exit status.
  • Orchestration selector covers the helper — sh/kvm.sh joins the root-orchestration regex, so a helper-only edit forces the full gate.
  • Dispatcher fixture stubs the probe — sh/dispatch-query.test.py writes a fixed stub in setUp; the new test_missing_kvm_refuses_flake_without_skipping_or_cancelling_other_modules covers both local check and strict ci (exit 1, names /dev/kvm, no (skipped), never runs nix flake check, still runs gh test). test_orchestration_and_all_select_work now covers sh/kvm.sh.
  • Quest retired — deletes quest/m0/ci/dispatch-test-kvm.md and its README entry.

Verified locally: all 31 sh/dispatch-query.test.py tests pass, including the three flake-mode tests and the new refusal test, and shellcheck sh/kvm.sh is clean.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

@kixelated
kixelated added this pull request to the merge queue Nov 2, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Nov 2, 2026
@moq-bot

moq-bot Bot commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

quest land watch: the merge queue removed this pull request at bda6834 because it conflicts with main (merge_conflict).

The land fixer works on it in the background, and the watcher re-enqueues it at the back of the queue once it is fixed or judged flaky, or hands it back here with what it needs.

(written by quest land watch)

@moq-bot
moq-bot Bot enabled auto-merge November 2, 2026 16:41
@moq-bot

moq-bot Bot commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

quest land watch: armed auto-merge on this pull request at 36309c5, so it joins the back of the merge queue once its required checks pass, with main at 04e9b36 merged in by the quest merge driver.

(written by quest land watch)

@moq-bot
moq-bot Bot added this pull request to the merge queue Nov 2, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Nov 2, 2026
@moq-bot

moq-bot Bot commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

quest land watch: the merge queue removed this pull request at 36309c5 because it conflicts with main (merge_conflict).

The land fixer works on it in the background, and the watcher re-enqueues it at the back of the queue once it is fixed or judged flaky, or hands it back here with what it needs.

(written by quest land watch)

@moq-bot
moq-bot Bot enabled auto-merge November 2, 2026 16:51
@moq-bot

moq-bot Bot commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

quest land watch: armed auto-merge on this pull request at 62c7d45, so it joins the back of the merge queue once its required checks pass, with main at 6944d3d merged in by the quest merge driver.

(written by quest land watch)

@moq-bot
moq-bot Bot added this pull request to the merge queue Nov 2, 2026
Merged via the queue into main with commit a607578 Nov 2, 2026
8 of 10 checks passed
@moq-bot
moq-bot Bot deleted the quest/m0/ci/dispatch-test-kvm branch November 2, 2026 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant