test(editors): add real-editor E2E smoke tests#458
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds shared CDB/test-data generation, Neovim and VS Code E2E smoke tests and fixtures, pixi editor environment/tasks, separates build and test in CMake CI (including editor job), wires Zed extension CI build, and updates English/Chinese editor setup docs and Quick Start links. ChangesEditor E2E Testing, Infrastructure, and CI/CD
Sequence Diagram(s)sequenceDiagram
participant Fixture as Test Fixture
participant Editor as Editor (VSCode/Neovim)
participant TestHarness as E2E Test Runner
participant LSP as clice LSP Server
participant CI as CI Job / pixi
Fixture->>Editor: open scenario workspace
Editor->>TestHarness: invoke test actions / extension
TestHarness->>LSP: start/initialize server (CLICE_EXECUTABLE)
LSP->>TestHarness: publishDiagnostics / indexing
TestHarness->>LSP: workspace/symbol, textDocument/hover, definition, completion
LSP->>TestHarness: provider responses
TestHarness->>CI: report results / upload artifacts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
5a51ed1 to
29f9d38
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a51ed1454
ℹ️ 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".
| path: | | ||
| build/${{ matrix.build_type }}/bin/ | ||
| build/${{ matrix.build_type }}/lib/ |
There was a problem hiding this comment.
Include Debug LLVM shared libraries in test artifacts
For the Ubuntu/macOS Debug matrix entries, the binaries uploaded here are not self-contained: cmake/llvm.cmake links non-Windows Debug builds against shared LLVM/clang libraries from ${LLVM_INSTALL_PATH}/lib via target_link_directories/target_link_libraries, but this artifact only carries build/Debug/bin and build/Debug/lib. The split test job runs in a fresh workspace after downloading only these paths, so unit_tests/clice can fail at startup with missing LLVM/clang dylibs/so files that were present in the original build job workspace.
Useful? React with 👍 / 👎.
| test-run = ["test"] | ||
| editor = ["editor", "node"] |
There was a problem hiding this comment.
Include build tools in CI test environments
The split test/editor jobs now install only the test-run/editor environments, but those environments omit the build feature that provides CMake, Ninja, clang, and lld. Integration tests and tests/editors/prepare.py still call tests.cdb.generate_cdb() for fixtures with CMakeLists.txt (for example module and index fixtures), which shells out to cmake -G Ninja with cmake/toolchain.cmake; on a fresh runner this can fail before clice is tested because those tools are no longer supplied by pixi.
Useful? React with 👍 / 👎.
| clice_dir = path / ".clice" | ||
| if clice_dir.exists(): | ||
| shutil.rmtree(clice_dir) |
There was a problem hiding this comment.
Clear the cache directory actually used by editor tests
When the real Neovim/VSCode clients start clice they do not override project.cache_dir, so the server default in Config::apply_defaults uses $XDG_CACHE_HOME or ~/.cache/clice/<workspace-hash> before falling back to workspace/.clice. Removing only path/.clice leaves the actual index/PCH cache intact; in the CI editor job, the VSCode run can reuse the cache populated by the preceding Neovim run for the same fixtures, masking indexing/cache regressions that these E2E tests are meant to catch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/editors/nvim/e2e.lua (1)
34-34: ⚡ Quick winScenario synchronization is a maintenance burden.
The comment directs maintainers to keep scenarios manually synchronized with
editors/vscode/src/test/e2e.test.ts. As fixtures evolve, this creates a risk of drift.Consider extracting the scenario definitions to a shared JSON or TOML file that both test suites can load, eliminating manual sync.
🤖 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/editors/nvim/e2e.lua` at line 34, The inline comment in tests/editors/nvim/e2e.lua forces manual sync with editors/vscode/src/test/e2e.test.ts; extract the shared scenario definitions into a single external data file (JSON or TOML) and update both test runners to load and parse that file instead of hardcoding scenarios. Create a new shared file (e.g., test-fixtures/scenarios.json or .toml) containing the scenario objects, add loading logic in the Neovim e2e loader (tests/editors/nvim/e2e.lua) and the VSCode test module (editors/vscode/src/test/e2e.test.ts) to read and validate the shared scenarios, and update any references to scenario names/fields so both suites consume the same structure to prevent drift.editors/vscode/src/test/e2e.test.ts (1)
106-143: ⚖️ Poor tradeoffTests depend on shared state from the first test.
The hover, definition, and completion tests reuse
documentandpositionassigned in the "server starts and publishes diagnostics" test. If that test fails or is skipped, these tests will fail with less informative messages. This is acceptable for a smoke test suite, but consider extracting a shared setup helper or noting the dependency chain in comments for future maintainers.🤖 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 `@editors/vscode/src/test/e2e.test.ts` around lines 106 - 143, The hover/definition/completion tests rely on shared variables document and position set in the "server starts and publishes diagnostics" test, which makes them brittle; add a dedicated setup that explicitly opens the test file and computes the test position before each of these tests. Implement a helper (e.g., openDocumentAndGetPosition) that uses vscode.workspace.openTextDocument and vscode.window.showTextDocument to set document and derives position (from the scenario marker or by searching the document text), then call that helper from a beforeEach or at the start of the "hover", "definition", and "completion" tests so each test initializes document and position independently of the "server starts and publishes diagnostics" test..github/workflows/test-cmake.yml (1)
249-292: ⚖️ Poor tradeoffConsider verifying the Neovim download URL integrity.
The job intentionally installs the latest stable Neovim release (unpinned) to catch new-editor-version regressions, downloading from
https://github.com/neovim/neovim/releases/download/stable/nvim-linux-x86_64.tar.gz. While the intent is clear and valuable, the download lacks checksum verification, exposing the CI to potential supply-chain attacks if the GitHub release is compromised.Consider adding checksum verification using a recent stable release's known hash, or fetching the
.sha256sumfile from the same release and verifying it:🔒 Optional: Add checksum verification
- name: Install neovim (stable) run: | curl -fsSL -o /tmp/nvim.tar.gz \ https://github.com/neovim/neovim/releases/download/stable/nvim-linux-x86_64.tar.gz + curl -fsSL -o /tmp/nvim.tar.gz.sha256sum \ + https://github.com/neovim/neovim/releases/download/stable/nvim-linux-x86_64.tar.gz.sha256sum + cd /tmp && sha256sum -c nvim.tar.gz.sha256sum sudo tar -xzf /tmp/nvim.tar.gz -C /opt echo "/opt/nvim-linux-x86_64/bin" >> "$GITHUB_PATH"🤖 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 @.github/workflows/test-cmake.yml around lines 249 - 292, The Neovim download in the test-editor job's "Install neovim (stable)" step lacks integrity verification; update that step to also download the corresponding checksum (e.g., .sha256 or .sha256sum) from the same release tag or a trusted source and verify the archive before extraction (use a SHA256 check like sha256sum -c and fail the job on mismatch), or embed a known-good hash and validate it after curl completes; modify the "Install neovim (stable)" run block to perform the checksum fetch/verify and exit non-zero on validation failure so the job aborts on tampered or corrupted downloads.
🤖 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 `@docs/en/guide/editors.md`:
- Around line 16-22: Update the Neovim section to explicitly tell users which
bundled file to copy and the exact target filename: replace the ambiguous "Copy
`doc/clice.lua`" with a clear instruction to copy the bundled clice.lua (the
Neovim config provided with the repo) into your Neovim config's lsp/ directory
as clice.lua, and keep the example API call vim.lsp.enable('clice') to show the
base-filename usage; locate the change in the Neovim section where the heading
"Neovim" and the code sample vim.lsp.enable('clice') appear.
In `@editors/vscode/src/test/e2e.test.ts`:
- Around line 64-71: The onDidChangeDiagnostics listener created in the
diagnostics Promise (use of vscode.languages.onDidChangeDiagnostics and const
listener) can leak if the test times out; modify the logic so the listener is
always disposed: create the Promise with resolve/reject, register the listener
as before and then ensure disposal in a finally block (or attach a timeout
handler) so listener.dispose() is called whether the event fires or the test
times out; reference the existing diagnostics promise and listener variable and
ensure uri matching logic stays the same while guaranteeing cleanup.
---
Nitpick comments:
In @.github/workflows/test-cmake.yml:
- Around line 249-292: The Neovim download in the test-editor job's "Install
neovim (stable)" step lacks integrity verification; update that step to also
download the corresponding checksum (e.g., .sha256 or .sha256sum) from the same
release tag or a trusted source and verify the archive before extraction (use a
SHA256 check like sha256sum -c and fail the job on mismatch), or embed a
known-good hash and validate it after curl completes; modify the "Install neovim
(stable)" run block to perform the checksum fetch/verify and exit non-zero on
validation failure so the job aborts on tampered or corrupted downloads.
In `@editors/vscode/src/test/e2e.test.ts`:
- Around line 106-143: The hover/definition/completion tests rely on shared
variables document and position set in the "server starts and publishes
diagnostics" test, which makes them brittle; add a dedicated setup that
explicitly opens the test file and computes the test position before each of
these tests. Implement a helper (e.g., openDocumentAndGetPosition) that uses
vscode.workspace.openTextDocument and vscode.window.showTextDocument to set
document and derives position (from the scenario marker or by searching the
document text), then call that helper from a beforeEach or at the start of the
"hover", "definition", and "completion" tests so each test initializes document
and position independently of the "server starts and publishes diagnostics"
test.
In `@tests/editors/nvim/e2e.lua`:
- Line 34: The inline comment in tests/editors/nvim/e2e.lua forces manual sync
with editors/vscode/src/test/e2e.test.ts; extract the shared scenario
definitions into a single external data file (JSON or TOML) and update both test
runners to load and parse that file instead of hardcoding scenarios. Create a
new shared file (e.g., test-fixtures/scenarios.json or .toml) containing the
scenario objects, add loading logic in the Neovim e2e loader
(tests/editors/nvim/e2e.lua) and the VSCode test module
(editors/vscode/src/test/e2e.test.ts) to read and validate the shared scenarios,
and update any references to scenario names/fields so both suites consume the
same structure to prevent drift.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e37eab4f-523d-4ad6-a048-1779f4c1d8b5
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
.github/workflows/main.yml.github/workflows/test-cmake.ymlcmake/package.cmakedocs/en/guide/editors.mddocs/en/guide/quick-start.mddocs/zh/guide/editors.mddocs/zh/guide/quick-start.mdeditors/vscode/.vscode-test.mjseditors/vscode/package.jsoneditors/vscode/pnpm-workspace.yamleditors/vscode/src/setting.tseditors/vscode/src/test/e2e.test.tspixi.tomlsrc/feature/hover.cppstylua.tomltests/cdb.pytests/conftest.pytests/editors/README.mdtests/editors/nvim/e2e.luatests/editors/prepare.pytests/integration/compilation/test_staleness.pytests/integration/modules/test_modules.pytests/replay.pytests/unit/feature/hover_tests.cpp
| ### Neovim | ||
|
|
||
| clice ships an LSP config for Neovim ≥ 0.11 in [`editors/nvim`](https://github.com/clice-io/clice/tree/main/editors/nvim). Copy `doc/clice.lua` into your config's `lsp/` directory, then enable it: | ||
|
|
||
| ```lua | ||
| vim.lsp.enable('clice') | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify Neovim LSP config paths and structure
# Expected: Find the Neovim LSP config file(s) and confirm the path users should copy from
# List all .lua files in editors/nvim and show their paths
echo "=== Neovim LSP config files ==="
fd -e lua . editors/nvim --type f 2>/dev/null | head -20
# Check if editors/nvim/doc/ exists and what it contains
echo "=== editors/nvim directory structure ==="
tree -L 2 editors/nvim 2>/dev/null || find editors/nvim -type f -name '*.lua' -o -type d | head -20Repository: clice-io/clice
Length of output: 368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Show editors/nvim/doc/clice.lua ==="
sed -n '1,200p' editors/nvim/doc/clice.lua
echo "=== Show editors/nvim/lua/clice-nvim/init.lua ==="
sed -n '1,200p' editors/nvim/lua/clice-nvim/init.lua
echo "=== Show editors/nvim/plugin/clice-nvim.lua ==="
sed -n '1,200p' editors/nvim/plugin/clice-nvim.lua
echo "=== Grep for vim.lsp.enable usage ==="
rg -n "vim\\.lsp\\.enable" -S editors/nvim || true
echo "=== Grep for lsp enable module naming ==="
rg -n "lsp\\.enable|clice['\"]" -S editors/nvim || trueRepository: clice-io/clice
Length of output: 1177
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Show editors/nvim/doc/clice.lua ==="
sed -n '1,200p' editors/nvim/doc/clice.lua
echo "=== Show editors/nvim/lua/clice-nvim/init.lua ==="
sed -n '1,200p' editors/nvim/lua/clice-nvim/init.lua
echo "=== Show editors/nvim/plugin/clice-nvim.lua ==="
sed -n '1,200p' editors/nvim/plugin/clice-nvim.lua
echo "=== Grep for vim.lsp.enable usage ==="
rg -n "vim\\.lsp\\.enable" -S editors/nvim || true
echo "=== Grep for lsp enable module naming ==="
rg -n "lsp\\.enable|clice['\"]" -S editors/nvim || trueRepository: clice-io/clice
Length of output: 1177
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Show docs/en/guide/editors.md lines 1-120 (focus around 16-22) ==="
nl -ba docs/en/guide/editors.md | sed -n '1,120p'
echo "=== Locate and show Neovim section in docs/en/guide/editors.md ==="
rg -n "### Neovim" -n docs/en/guide/editors.md
nl -ba docs/en/guide/editors.md | sed -n '12,60p'
echo "=== Search for vim.lsp.enable / lsp.enable in repo ==="
rg -n "vim\\.lsp\\.enable|lsp\\.enable\\s*\\(" -S .
echo "=== Search for definition of lsp.enable or vim.lsp.enable ==="
rg -n "function\\s+.*lsp\\.enable|enable\\s*=\\s*function.*lsp" -S . || true
echo "=== Show matching Chinese guide if present ==="
# Try common path; if missing, rg for 'vim.lsp.enable' or '编辑器' etc.
if [ -f docs/zh/guide/editors.md ]; then
echo "=== Show docs/zh/guide/editors.md lines 1-120 ==="
nl -ba docs/zh/guide/editors.md | sed -n '1,120p'
nl -ba docs/zh/guide/editors.md | sed -n '12,60p'
else
echo "docs/zh/guide/editors.md not found; searching for 'Neovim' in docs/zh/guide ==="
rg -n "Neovim|nvim" docs/zh/guide || true
fiRepository: clice-io/clice
Length of output: 190
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Show docs/en/guide/editors.md lines 1-120 (focus around 16-22) ==="
nl -ba docs/en/guide/editors.md | sed -n '1,120p'
echo "=== Locate and show Neovim section in docs/en/guide/editors.md ==="
rg -n "### Neovim" docs/en/guide/editors.md
nl -ba docs/en/guide/editors.md | sed -n '12,60p'
echo "=== Search for vim.lsp.enable / lsp.enable in repo ==="
rg -n "vim\\.lsp\\.enable|lsp\\.enable\\s*\\(" -S .
echo "=== Search for definition of lsp.enable or vim.lsp.enable ==="
rg -n "function\\s+.*lsp\\.enable|enable\\s*=\\s*function.*lsp" -S . || true
echo "=== Show matching Chinese guide if present ==="
if [ -f docs/zh/guide/editors.md ]; then
echo "=== Show docs/zh/guide/editors.md lines 1-120 ==="
nl -ba docs/zh/guide/editors.md | sed -n '1,120p'
nl -ba docs/zh/guide/editors.md | sed -n '12,60p'
else
echo "docs/zh/guide/editors.md not found; searching for 'Neovim' in docs/zh/guide ==="
rg -n "Neovim|nvim" docs/zh/guide || true
fiRepository: clice-io/clice
Length of output: 190
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Show docs/en/guide/editors.md lines 1-120 (focus around 16-22) ==="
awk '{printf "%d\t%s\n", NR, $0}' docs/en/guide/editors.md | sed -n '1,120p'
echo "=== Locate and show Neovim section in docs/en/guide/editors.md ==="
awk '{printf "%d\t%s\n", NR, $0}' docs/en/guide/editors.md | sed -n '10,60p'
echo "=== Search for vim.lsp.enable / lsp.enable in repo ==="
rg -n "vim\\.lsp\\.enable|lsp\\.enable\\s*\\(" -S .
echo "=== Search for definition of lsp.enable or vim.lsp.enable (any function or assignment) ==="
rg -n "(function\\s+.*lsp\\.enable|vim\\.lsp\\.enable\\s*=|lsp\\.enable\\s*=)" -S . || true
echo "=== Check if docs/zh/guide/editors.md exists ==="
if [ -f docs/zh/guide/editors.md ]; then
echo "FOUND: docs/zh/guide/editors.md"
awk '{printf "%d\t%s\n", NR, $0}' docs/zh/guide/editors.md | sed -n '1,120p'
awk '{printf "%d\t%s\n", NR, $0}' docs/zh/guide/editors.md | sed -n '10,60p'
else
echo "NOT FOUND: docs/zh/guide/editors.md"
echo "=== Search for editors guide (zh) ==="
fd editors.md docs/zh -t f 2>/dev/null || true
echo "=== Search for 'vim.lsp.enable' in docs/zh ==="
rg -n "vim\\.lsp\\.enable" -S docs/zh || true
fiRepository: clice-io/clice
Length of output: 7997
🌐 Web query:
Neovim 0.11 vim.lsp.enable('name') documentation lsp/ directory config file name
💡 Result:
In Neovim 0.11+, the function vim.lsp.enable('name') is used to auto-activate a Language Server Protocol (LSP) configuration [1][2]. The name argument corresponds to the base filename of an LSP configuration file located within an lsp/ directory in your runtime path [3][4]. Configuration File Structure and Location Neovim searches for LSP configuration files in lsp/ directories located anywhere in your 'runtimepath' [4][5]. To create a custom configuration for a server, you should create an lsp/ directory at the root of your Neovim configuration directory (typically ~/.config/nvim/lsp/) [3][4]. For example, to configure a server named 'lua_ls': 1. Create the file: ~/.config/nvim/lsp/lua_ls.lua [3]. 2. Define the configuration by returning a Lua table in that file [4][6]. Example content for ~/.config/nvim/lsp/lua_ls.lua: return { cmd = { 'lua-language-server' }, filetypes = { 'lua' }, root_markers = { '.git', '.lua-language-server.json' }, } After creating the file, you can enable it in your init.lua using: vim.lsp.enable('lua_ls') [1][7] Key Details - Priority: If you need to override default configurations provided by plugins, you can place your files in an after/lsp/ directory (e.g., ~/.config/nvim/after/lsp/lua_ls.lua), as these are loaded after the standard lsp/ directories [8][6]. - Integration: This system replaces the older require('lspconfig') framework [9][7]. Neovim automatically merges configurations defined in lsp/ files with those specified via the vim.lsp.config function [9][4]. - Verification: You can verify that your configuration is active by running the command:checkhealth vim.lsp [7][4].
Citations:
- 1: https://github.com/neovim/neovim/blob/4aa8969d/runtime/doc/lsp.txt
- 2: https://neovim.io/doc/user/lsp/
- 3: https://tduyng.com/blog/neovim-lsp-native/
- 4: https://github.com/neovim/nvim-lspconfig?tab=readme-ov-file
- 5: https://github.com/neovim/nvim-lsp/
- 6: https://www.mintlify.com/neovim/neovim/lsp/configuration
- 7: https://github.com/neovim/nvim-lspconfig/blob/master/README.md
- 8: https://github.com/neovim/neovim/blob/44d1f82f/runtime/doc/lsp.txt
- 9: https://github.com/neovim/nvim-lspconfig/blob/master/doc/lspconfig.txt
Clarify Neovim doc source path; vim.lsp.enable('clice') API usage matches the repo config filename.
editors/nvim/doc/clice.luaexists, and copying it into your Neovim config’slsp/directory aslsp/clice.luais consistent withvim.lsp.enable('clice')(Neovim uses the base filename of thelsp/*.luaconfig).- The guides are mostly correct, but “Copy
doc/clice.lua” is still a bit ambiguous—prefer explicitly statingeditors/nvim/doc/clice.lua(or “copy it into yourlsp/asclice.lua”) in bothdocs/en/guide/editors.mdanddocs/zh/guide/editors.md(lines 16–22).
🤖 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 `@docs/en/guide/editors.md` around lines 16 - 22, Update the Neovim section to
explicitly tell users which bundled file to copy and the exact target filename:
replace the ambiguous "Copy `doc/clice.lua`" with a clear instruction to copy
the bundled clice.lua (the Neovim config provided with the repo) into your
Neovim config's lsp/ directory as clice.lua, and keep the example API call
vim.lsp.enable('clice') to show the base-filename usage; locate the change in
the Neovim section where the heading "Neovim" and the code sample
vim.lsp.enable('clice') appear.
| const diagnostics = new Promise<void>((resolve) => { | ||
| const listener = vscode.languages.onDidChangeDiagnostics((event) => { | ||
| if (event.uris.some((u) => u.toString() === uri.toString())) { | ||
| listener.dispose(); | ||
| resolve(); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Diagnostics listener may leak on test timeout.
The onDidChangeDiagnostics listener disposes itself when the event fires, but if the test times out before diagnostics arrive, the listener remains registered. Wrap the promise in a try/finally or use this.timeout() handler to guarantee cleanup.
🧹 Proposed fix to guarantee listener disposal
- const diagnostics = new Promise<void>((resolve) => {
+ let listener: vscode.Disposable | undefined;
+ const diagnostics = new Promise<void>((resolve) => {
- const listener = vscode.languages.onDidChangeDiagnostics((event) => {
+ listener = vscode.languages.onDidChangeDiagnostics((event) => {
if (event.uris.some((u) => u.toString() === uri.toString())) {
- listener.dispose();
+ listener?.dispose();
+ listener = undefined;
resolve();
}
});
});
// Opening the C++ document activates the extension, which starts clice.
document = await vscode.workspace.openTextDocument(uri);
await vscode.window.showTextDocument(document);
const offset = document.getText().indexOf(scenario.symbol);
assert.ok(offset >= 0, `symbol not found: ${scenario.symbol}`);
position = document.positionAt(offset);
- await diagnostics;
+ try {
+ await diagnostics;
+ } finally {
+ listener?.dispose();
+ }📝 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 diagnostics = new Promise<void>((resolve) => { | |
| const listener = vscode.languages.onDidChangeDiagnostics((event) => { | |
| if (event.uris.some((u) => u.toString() === uri.toString())) { | |
| listener.dispose(); | |
| resolve(); | |
| } | |
| }); | |
| }); | |
| let listener: vscode.Disposable | undefined; | |
| const diagnostics = new Promise<void>((resolve) => { | |
| listener = vscode.languages.onDidChangeDiagnostics((event) => { | |
| if (event.uris.some((u) => u.toString() === uri.toString())) { | |
| listener?.dispose(); | |
| listener = undefined; | |
| resolve(); | |
| } | |
| }); | |
| }); | |
| // Opening the C++ document activates the extension, which starts clice. | |
| document = await vscode.workspace.openTextDocument(uri); | |
| await vscode.window.showTextDocument(document); | |
| const offset = document.getText().indexOf(scenario.symbol); | |
| assert.ok(offset >= 0, `symbol not found: ${scenario.symbol}`); | |
| position = document.positionAt(offset); | |
| try { | |
| await diagnostics; | |
| } finally { | |
| listener?.dispose(); | |
| } |
🤖 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 `@editors/vscode/src/test/e2e.test.ts` around lines 64 - 71, The
onDidChangeDiagnostics listener created in the diagnostics Promise (use of
vscode.languages.onDidChangeDiagnostics and const listener) can leak if the test
times out; modify the logic so the listener is always disposed: create the
Promise with resolve/reject, register the listener as before and then ensure
disposal in a finally block (or attach a timeout handler) so listener.dispose()
is called whether the event fires or the test times out; reference the existing
diagnostics promise and listener variable and ensure uri matching logic stays
the same while guaranteeing cleanup.
Run real editors headlessly against the built clice binary on two fixtures (hello_world and a C++20 modules project): start, didOpen + first diagnostics, hover, definition across the module boundary, and completion, asserting well-formed non-empty responses only. - nvim: editors/nvim/tests/e2e.lua driven by nvim -l, starting clice through the shipped LSP config in doc/clice.lua. - vscode: e2e.test.ts via @vscode/test-cli; pretest now also compiles tests (out/test was never built before, so the sample suite never ran). CLICE_EXECUTABLE env overrides the executable setting. - pixi: new editor env with editor-prepare/nvim-e2e/vscode-e2e/ editor-test tasks; CDB helpers extracted from conftest to tests/cdb so tests/prepare.py works without pytest deps; pnpm pinned to 10.x with onlyBuiltDependencies moved to pnpm-workspace.yaml. - stylua.toml moved to the repo root so all Lua shares one style.
Native platforms now mirror the cross-compile pattern: build jobs upload bin/ + lib/ artifacts, test jobs download them and run the suites in the test-run env. A new test-editor job installs the latest stable neovim and VSCode (unpinned on purpose, to catch breakage from new editor releases) and runs the editor E2E tests against the ubuntu RelWithDebInfo artifact. The Zed extension gets a wasm32-wasip1 compile check gated on editors/zed changes.
Per-editor instructions: official plugins (VSCode, Neovim, Zed) plus generic LSP client snippets for Helix, Emacs, Sublime Text, Kate and Vim. quick-start's empty editor sections now link here. The dev test-and-debug page documents how to run the editor E2E tests.
29f9d38 to
8c00f47
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/test-cmake.yml (1)
249-293:⚠️ Potential issue | 🟡 MinorConfirm the
editorpixi environment includes Node.js + pnpm
pixi.tomldefineseditor = ["editor", "node"], and thenodefeature declaresnodejs >=20andpnpm >=10,<11, so theeditorenvironment provides the Node/pnpm toolchain required for pnpm-basedvscode-e2esteps.🤖 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 @.github/workflows/test-cmake.yml around lines 249 - 293, Verify that the pixi environment named "editor" (as referenced in the workflow's step using "pixi run -e editor ...") actually includes the Node/pnpm toolchain by checking pixi.toml where the editor environment should include the "node" feature; ensure the "node" feature declares nodejs >=20 and pnpm >=10,<11 so that the VSCode E2E step (vscode-e2e) can run with pnpm and the Neovim E2E step (nvim-e2e) is unaffected.
🤖 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/main.yml:
- Around line 110-122: Update the "zed" GitHub Actions job to harden against
supply-chain and credential leakage: replace the moving tags used in the steps
that call actions/checkout@v4 and Swatinem/rust-cache@v2 with their
corresponding immutable commit SHAs, set the actions/checkout step option
persist-credentials: false, constrain the job's permissions by adding an
explicit permissions: block with only the minimal scopes needed, and modify the
rust-cache usage (Swatinem/rust-cache call) to use a safer cache mode or
restrict cache restore/write boundaries so cached artifacts cannot be used to
reintroduce untrusted binaries (treat cache as tainted by default).
In @.github/workflows/test-cmake.yml:
- Around line 278-284: The cache key currently uses the per-run variable
github.run_id so it never hits previous caches; update the actions/cache@v4 step
(the "Restore VSCode download cache" block) to use a stable key instead of
vscode-test-${{ runner.os }}-${{ github.run_id }} — for example use
vscode-test-${{ runner.os }} or include a stable hash of your VSCode test
configuration (computed in a prior step) like vscode-test-${{ runner.os }}-${{
steps.compute_vscode_hash.outputs.hash }}; keep the restore-keys prefix
(vscode-test-${{ runner.os }}-) for fallback so caches can be reused across
runs.
In `@editors/nvim/tests/e2e.lua`:
- Around line 55-59: The dofile call loading plugin_root .. '/doc/clice.lua' can
throw; replace the direct dofile with a protected load using pcall (or loadfile
+ pcall) around the existing dofile invocation, assign to config only on
success, and surface a clear test failure message if loading/parsing fails;
specifically update the code around the plugin_root variable and the config =
dofile(...) line to pcall the load, check the boolean result, and if false
assert or error with a message like "failed to load clice config: <error>" so
tests fail with a readable cause.
---
Outside diff comments:
In @.github/workflows/test-cmake.yml:
- Around line 249-293: Verify that the pixi environment named "editor" (as
referenced in the workflow's step using "pixi run -e editor ...") actually
includes the Node/pnpm toolchain by checking pixi.toml where the editor
environment should include the "node" feature; ensure the "node" feature
declares nodejs >=20 and pnpm >=10,<11 so that the VSCode E2E step (vscode-e2e)
can run with pnpm and the Neovim E2E step (nvim-e2e) is unaffected.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cc902d53-343e-4e65-be42-b2cb8d253f41
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
.github/workflows/main.yml.github/workflows/test-cmake.ymldocs/en/dev/test-and-debug.mddocs/en/guide/editors.mddocs/en/guide/quick-start.mddocs/zh/dev/test-and-debug.mddocs/zh/guide/editors.mddocs/zh/guide/quick-start.mdeditors/nvim/tests/e2e.luaeditors/vscode/.vscode-test.mjseditors/vscode/package.jsoneditors/vscode/pnpm-workspace.yamleditors/vscode/src/setting.tseditors/vscode/src/test/e2e.test.tspixi.tomlstylua.tomltests/cdb.pytests/conftest.pytests/integration/compilation/test_staleness.pytests/integration/modules/test_modules.pytests/prepare.py
✅ Files skipped from review due to trivial changes (4)
- editors/vscode/src/setting.ts
- docs/en/guide/quick-start.md
- docs/zh/guide/editors.md
- docs/en/guide/editors.md
🚧 Files skipped from review as they are similar to previous changes (8)
- docs/zh/guide/quick-start.md
- editors/vscode/pnpm-workspace.yaml
- tests/integration/compilation/test_staleness.py
- editors/vscode/package.json
- pixi.toml
- editors/vscode/.vscode-test.mjs
- tests/conftest.py
- editors/vscode/src/test/e2e.test.ts
| zed: | ||
| needs: changes | ||
| if: ${{ needs.changes.outputs.zed == 'true' }} | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: Swatinem/rust-cache@v2 | ||
| with: | ||
| workspaces: editors/zed | ||
| - name: Build Zed extension | ||
| run: | | ||
| rustup target add wasm32-wasip1 | ||
| cargo build --manifest-path editors/zed/Cargo.toml --target wasm32-wasip1 --release |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for security policy documents and action pinning requirements
rg -i "pin.*action|action.*pin|commit.*sha|security.*policy" --glob '*.md' --glob '.github/**'Repository: clice-io/clice
Length of output: 40
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== main.yml uses =="
sed -n '1,220p' .github/workflows/main.yml | nl -ba | sed -n '80,170p'
echo
echo "== Find checkout/rust-cache usages =="
rg -n "uses:\s*actions/checkout@|uses:\s*Swatinem/rust-cache@" .github/workflows/main.yml
echo
echo "== Find persist-credentials usage =="
rg -n "persist-credentials" .github/workflows/main.yml .github/workflows/*.yml .github/**/*.yml 2>/dev/null || true
echo
echo "== Find permissions blocks =="
rg -n "^\s*permissions:" .github/workflows/main.yml .github/workflows/*.yml .github/**/*.yml 2>/dev/null || true
echo
echo "== Search for action pinning / policy / zizmor config in repo =="
rg -n -S "zizmor|pin.*action|action.*pin|commit.*sha|blanket policy|security policy" .github -g'*.yml' -g'*.yaml' -g'*.md' || trueRepository: clice-io/clice
Length of output: 139
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== main.yml (lines 1-220) =="
sed -n '1,220p' .github/workflows/main.yml
echo
echo "== Find checkout/rust-cache usages in main.yml =="
rg -n "uses:\s*actions/checkout@|uses:\s*Swatinem/rust-cache@" .github/workflows/main.yml
echo
echo "== Find persist-credentials usage =="
rg -n "persist-credentials" .github/workflows .github/**/*.yml .github/**/*.yaml 2>/dev/null || true
echo
echo "== Find permissions blocks =="
rg -n "^\s*permissions:" .github/workflows .github/**/*.yml .github/**/*.yaml 2>/dev/null || true
echo
echo "== Search for action pinning / policy / zizmor config in .github =="
rg -n -S "zizmor|pin.*action|action.*pin|commit.*sha|blanket policy|security policy" .github -g'*.yml' -g'*.yaml' -g'*.md' || true
echo
echo "== List workflow files =="
ls -la .github/workflowsRepository: clice-io/clice
Length of output: 6416
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Any workflow-level permissions: =="
rg -n "^\s*permissions:" .github/workflows/*.yml .github/workflows/*.yaml || true
echo
echo "== All actions/checkout@ usages =="
rg -n "uses:\s*actions/checkout@" .github/workflows || true
echo
echo "== All Swatinem/rust-cache@ usages =="
rg -n "uses:\s*Swatinem/rust-cache@" .github/workflows || true
echo
echo "== Any persist-credentials usage across workflows =="
rg -n "persist-credentials" .github/workflows || true
echo
echo "== Any actions pinned to commit SHA (basic heuristics) =="
rg -n "uses:\s*([^/@]+)/([^/@]+)@([0-9a-f]{40})" .github/workflows || true
echo
echo "== Check if main.yml has top-level permissions other than jobs.permissions =="
rg -n "^\s*permissions:" .github/workflows/main.yml || trueRepository: clice-io/clice
Length of output: 2139
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for zizmor in repo =="
rg -n -S "zizmor" . || true
echo
echo "== Check for workflow-level top-level permissions (no indentation) =="
rg -n "^permissions:" .github/workflows || true
echo
echo "== Check if any workflow sets default permissions globally =="
rg -n "permissions:\s*$" .github/workflows || trueRepository: clice-io/clice
Length of output: 620
🌐 Web query:
actions/checkout persist-credentials default value
💡 Result:
The input actions/checkout@... persist-credentials has a default value of true. [1][2] This means the action will persist the auth token/SSH credentials in the local git configuration (enabling authenticated git commands) unless you explicitly set persist-credentials: false. [2]
Citations:
- 1: https://github.com/actions/checkout/blob/34e114876b0b11c390a56381ad16ebd13914f8d5/action.yml
- 2: https://github.com/actions/checkout/?tab=readme-ov-file
🌐 Web query:
Swatinem/rust-cache security cache poisoning risk
💡 Result:
Swatinem/rust-cache itself is not known (from its public advisories) to have a specific, standalone “cache poisoning” vulnerability. However, it can participate in cache-poisoning attacks in the same way as any GitHub Actions cache-based workflow, because it uses GitHub’s cache mechanism (via actions/cache) to restore previously saved content; if an attacker can write a malicious cache entry that a later, more-privileged workflow will restore, that can lead to supply-chain compromise. 1) What “cache poisoning” means in this context A cache poisoning scenario is when malicious content is stored in a shared cache and later restored by a different job/workflow that trusts that cached content; this can allow malicious dependencies/tooling to be included in build/release artifacts. Guidance for GitHub Actions notes that steps which can publish artifacts should not write cache (and should instead use read-only cache/lookup-only controls) to avoid this risk [1]. 2) How Swatinem/rust-cache behaves relevant to the risk - rust-cache is a GitHub Action that caches Rust/Cargo artifacts (primarily dependency caches and optionally ~/.cargo/bin and build outputs) [2]. - In its README, rust-cache documents that before persisting the cache it cleans ~/.cargo/bin (by default, removing files that existed before the action ran) and that you can disable that cleanup with cache-bin: "false" [2]. This cleanup behavior is an operational detail; it does not eliminate the broader cache poisoning threat model (malicious cached content restoring into later runs). 3) Evidence that cache-related security guidance applies to this action - A published security rule/example explicitly lists Swatinem/rust-cache as a cache-aware action and instructs that for publishing/release workflows you should disable cache writes (e.g., use lookup-only/read-only cache mode) to reduce cache poisoning risk [1]. 4) Security advisories / known vulns for the action - The project’s GitHub “Security” page reports that there aren’t any published security advisories [3]. That suggests no widely disclosed, action-specific cache poisoning CVE/advisory is publicly documented. 5) Practical mitigations (how to reduce cache poisoning risk when using rust-cache) - Use read-only/lookup-only mode (or avoid saving cache) in jobs that publish artifacts/releases. The Datadog cache poisoning guidance uses lookup-only for cache actions including Swatinem/rust-cache in publishing workflows [1]. - Avoid allowing untrusted workflows to write to caches that are later restored by trusted workflows. General cache poisoning write/restore separation is the core mitigation described in the security guidance [1]. Bottom line: No public, action-specific “Swatinem/rust-cache cache poisoning” advisory was found, but rust-cache can be part of the general GitHub Actions cache poisoning threat if an attacker can influence cache contents that later privileged workflows restore. The recommended countermeasure is to ensure trusted/publishing workflows do not write cache and use read-only cache/lookup-only behavior [1].
Citations:
- 1: https://docs.datadoghq.com/security/code_security/iac_security/iac_rules/cicd-github-cache-poisoning/
- 2: https://github.com/Swatinem/rust-cache/blob/11da8522/README.md
- 3: https://github.com/Swatinem/rust-cache/security
Harden the zed CI job against supply-chain and credential leakage
- Pin
actions/checkout@v4andSwatinem/rust-cache@v2to immutable commit SHAs instead of moving tags (@v4/@v2) to reduce supply-chain risk from tag retargeting. - Add
persist-credentials: falsetoactions/checkout(the action’s default istrue, so credentials/tokens are persisted unless explicitly disabled). - Treat the Rust cache as part of the cache-poisoning threat model: while no action-specific advisory is known, cache restore can reintroduce malicious artifacts if cache contents are attacker-influenced; use safer cache modes / ensure trusted cache write boundaries for privileged contexts.
- Scope
permissions:for thezedjob (it currently has no explicit permissions block and will inherit broader defaults).
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 115-115: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 110-122: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
[error] 115-115: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 116-116: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 116-116: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): enables caching by default
(cache-poisoning)
🤖 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 @.github/workflows/main.yml around lines 110 - 122, Update the "zed" GitHub
Actions job to harden against supply-chain and credential leakage: replace the
moving tags used in the steps that call actions/checkout@v4 and
Swatinem/rust-cache@v2 with their corresponding immutable commit SHAs, set the
actions/checkout step option persist-credentials: false, constrain the job's
permissions by adding an explicit permissions: block with only the minimal
scopes needed, and modify the rust-cache usage (Swatinem/rust-cache call) to use
a safer cache mode or restrict cache restore/write boundaries so cached
artifacts cannot be used to reintroduce untrusted binaries (treat cache as
tainted by default).
Source: Linters/SAST tools
| - name: Restore VSCode download cache | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: editors/vscode/.vscode-test | ||
| key: vscode-test-${{ runner.os }}-${{ github.run_id }} | ||
| restore-keys: | | ||
| vscode-test-${{ runner.os }}- |
There was a problem hiding this comment.
VSCode cache key uses github.run_id, preventing cache hits.
github.run_id is unique per workflow run, so the cache key will never match a previous run's cache. While restore-keys provides a fallback prefix match, every run creates a new cache entry and immediately overwrites it, wasting storage and defeating the purpose of caching.
Use a stable identifier (e.g., a hash of the VSCode test configuration or simply omit the run-specific suffix):
Proposed fix
- name: Restore VSCode download cache
uses: actions/cache@v4
with:
path: editors/vscode/.vscode-test
- key: vscode-test-${{ runner.os }}-${{ github.run_id }}
+ key: vscode-test-${{ runner.os }}-${{ hashFiles('editors/vscode/.vscode-test.mjs') }}
restore-keys: |
vscode-test-${{ runner.os }}-🧰 Tools
🪛 zizmor (1.25.2)
[error] 279-279: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/test-cmake.yml around lines 278 - 284, The cache key
currently uses the per-run variable github.run_id so it never hits previous
caches; update the actions/cache@v4 step (the "Restore VSCode download cache"
block) to use a stable key instead of vscode-test-${{ runner.os }}-${{
github.run_id }} — for example use vscode-test-${{ runner.os }} or include a
stable hash of your VSCode test configuration (computed in a prior step) like
vscode-test-${{ runner.os }}-${{ steps.compute_vscode_hash.outputs.hash }}; keep
the restore-keys prefix (vscode-test-${{ runner.os }}-) for fallback so caches
can be reused across runs.
| local plugin_root = vim.fn.fnamemodify(arg[0], ':p:h:h') | ||
| local config = dofile(plugin_root .. '/doc/clice.lua') | ||
| config.name = 'clice' | ||
| config.cmd = { clice_path, 'server' } | ||
| config.root_dir = fixture_dir |
There was a problem hiding this comment.
Add error handling for dofile to catch missing config.
If plugin_root .. '/doc/clice.lua' does not exist or contains invalid Lua, dofile will error and the test will fail with a cryptic message. Consider wrapping it in pcall to provide a clearer failure message.
🛡️ Proposed fix with error handling
local plugin_root = vim.fn.fnamemodify(arg[0], ':p:h:h')
-local config = dofile(plugin_root .. '/doc/clice.lua')
+local config_path = plugin_root .. '/doc/clice.lua'
+local ok, config = pcall(dofile, config_path)
+if not ok then
+ fail('failed to load clice.lua: ' .. config)
+end
config.name = 'clice'🤖 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 `@editors/nvim/tests/e2e.lua` around lines 55 - 59, The dofile call loading
plugin_root .. '/doc/clice.lua' can throw; replace the direct dofile with a
protected load using pcall (or loadfile + pcall) around the existing dofile
invocation, assign to config only on success, and surface a clear test failure
message if loading/parsing fails; specifically update the code around the
plugin_root variable and the config = dofile(...) line to pcall the load, check
the boolean result, and if false assert or error with a message like "failed to
load clice config: <error>" so tests fail with a readable cause.
Two failures surfaced by the build/test split, where tests now run in the test-run env on a machine that never built: - ToolchainTests.GCC queries the g++ from PATH and compiles <print>; the runner's system gcc is too old. Pin gcc 14.2.0 into the test feature on linux-64, matching what the build env provided before. - Debug prebuilt LLVM links shared: binaries reference build/Debug/.llvm/lib via build-tree RPATH, which did not exist on the test runner. Include the package's shared libraries in the artifact so they land at the same path; static packages match nothing.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/test-cmake.yml (1)
94-96:⚠️ Potential issue | 🟠 MajormacOS cross-build runs with Pixi
defaultinstead ofcross-macos-x64
- The
macos-15matrix row (x86_64-apple-darwin) doesn’t setpixi_env, so.github/actions/setup-pixiactivates${{ matrix.pixi_env || 'default' }}→default(["build", "test"]).pixi.tomldefinescross-macos-x64, which includes thecross-macos-x64feature and runsscripts/activate_cross_macos.sh(clears host conda flags). With the current matrix, that activation is skipped for this job.Expected result: set
pixi_env: cross-macos-x64for themacos-15row (or remove/rework thecross-macos-x64environment/feature if it’s truly unnecessary).🤖 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 @.github/workflows/test-cmake.yml around lines 94 - 96, The macOS matrix row "macos-15" is missing pixi_env so setup-pixi falls back to the default environment instead of the intended cross-macos-x64; update the matrix row for macos-15 in the workflow to include pixi_env: cross-macos-x64 so .github/actions/setup-pixi activates the cross-macos-x64 environment (which corresponds to the cross-macos-x64 entry in pixi.toml and runs scripts/activate_cross_macos.sh), or alternatively remove/rework the cross-macos-x64 definition from pixi.toml if that cross-activation is not required.
🤖 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/test-cmake.yml:
- Line 66: Replace the floating ref uses: actions/upload-artifact@v4 with a
pinned commit SHA for the upload-artifact action; locate the occurrences of the
string "uses: actions/upload-artifact@v4" in the workflow and replace both
instances with "uses: actions/upload-artifact@<FULL_COMMIT_SHA>" (use the
specific commit SHA from the actions/upload-artifact repository you want to pin
to) so the workflow uses a stable, immutable reference.
---
Outside diff comments:
In @.github/workflows/test-cmake.yml:
- Around line 94-96: The macOS matrix row "macos-15" is missing pixi_env so
setup-pixi falls back to the default environment instead of the intended
cross-macos-x64; update the matrix row for macos-15 in the workflow to include
pixi_env: cross-macos-x64 so .github/actions/setup-pixi activates the
cross-macos-x64 environment (which corresponds to the cross-macos-x64 entry in
pixi.toml and runs scripts/activate_cross_macos.sh), or alternatively
remove/rework the cross-macos-x64 definition from pixi.toml if that
cross-activation is not required.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b47fb330-3fc4-4404-b40f-783ec0a31848
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
.github/workflows/test-cmake.ymlpixi.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- pixi.toml
| # they must exist at the same path on the test runner. Static packages | ||
| # simply match nothing here. | ||
| - name: Upload binaries | ||
| uses: actions/upload-artifact@v4 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/test-cmake.yml"
echo "== File: $FILE =="
nl -ba "$FILE" | sed -n '50,90p'
echo
echo "== Find all actions/upload-artifact occurrences =="
rg -n "uses:\s*actions/upload-artifact@" "$FILE" || true
echo
echo "== Check if other actions are pinned to SHAs in this workflow =="
rg -n "uses:\s*actions/" "$FILE" | head -n 50Repository: clice-io/clice
Length of output: 164
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/test-cmake.yml"
echo "== File: $FILE =="
echo "(lines 55-80)"
cat -n "$FILE" | sed -n '55,80p'
echo
echo "== Find all actions/upload-artifact occurrences =="
rg -n "uses:\s*actions/upload-artifact@" "$FILE" || true
echo
echo "== Check actions/* refs used in this workflow =="
rg -n "uses:\s*actions/" "$FILE" | head -n 200 || trueRepository: clice-io/clice
Length of output: 1984
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/test-cmake.yml"
echo "== SHA-pinned action refs in $FILE (pattern: @<40hex>) =="
rg -n "uses:\s*[^ ]+@[0-9a-f]{40}$" "$FILE" || true
echo
echo "== Any uses refs that look like SHAs (first 80 chars) =="
rg -n "uses:\s*[^ ]+@" "$FILE" | head -n 200Repository: clice-io/clice
Length of output: 724
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Find any SHA-pinned GitHub actions across .github/workflows =="
rg -n "uses:\s*[^ ]+@[0-9a-f]{40}$" .github/workflows || true
echo
echo "== List all uses: refs in test-cmake.yml (for context) =="
rg -n "uses:\s*actions/" .github/workflows/test-cmake.ymlRepository: clice-io/clice
Length of output: 706
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "zizmor|action pin|pin.*actions|pinning|actions/.*`@v`" .github . || trueRepository: clice-io/clice
Length of output: 2324
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Files potentially related to Zizmor =="
find . -maxdepth 4 -type f \( -iname '*zizmor*' -o -iname '*pin*action*' -o -iname '*action*pin*' \) 2>/dev/null | sort || true
echo
echo "== Search for 'zizmor' text across repo =="
rg -n "zizmor" . || true
echo
echo "== Search for common pinning config keys =="
rg -n "pin.*actions|actions.*pin|pinned.*actions|github.*actions.*pin|pin.*github" .github . || trueRepository: clice-io/clice
Length of output: 189
Pin actions/upload-artifact to a commit SHA.
.github/workflows/test-cmake.yml uses the floating ref actions/upload-artifact@v4 (lines 66 and 142); pin it to a full commit SHA to avoid unexpected upstream changes.
🧰 Tools
🪛 zizmor (1.25.2)
[error] 66-66: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/test-cmake.yml at line 66, Replace the floating ref uses:
actions/upload-artifact@v4 with a pinned commit SHA for the upload-artifact
action; locate the occurrences of the string "uses: actions/upload-artifact@v4"
in the workflow and replace both instances with "uses:
actions/upload-artifact@<FULL_COMMIT_SHA>" (use the specific commit SHA from the
actions/upload-artifact repository you want to pin to) so the workflow uses a
stable, immutable reference.
Source: Linters/SAST tools
Debug builds are ASAN-instrumented and macOS links the runtime as a dylib whose @rpath entries point into the env that built it, so test jobs running downloaded artifacts abort with 'Library not loaded'. Mirror activate_asan.bat: an activation script for the test feature points DYLD_LIBRARY_PATH at the activated env's clang resource dir. compiler-rt was already a transitive dep of clang on macOS; pin it explicitly since the test env now relies on it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e25fc4201
ℹ️ 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".
| build/${{ matrix.build_type }}/.llvm/lib/*.so* | ||
| build/${{ matrix.build_type }}/.llvm/lib/*.dylib | ||
| build/${{ matrix.build_type }}/.llvm/bin/*.dll |
There was a problem hiding this comment.
Upload the hidden LLVM runtime directory
Although this revision adds explicit .llvm globs, actions/upload-artifact@v4 documents include-hidden-files as defaulting to false, so files under the hidden build/${{ matrix.build_type }}/.llvm directory are still excluded from the artifact. For Debug Ubuntu/macOS builds, the comment here says the test binaries rely on those shared LLVM/Clang libraries via build-tree RPATH, so the split test runner will download bin/ and lib/ without the needed .llvm/lib dylibs/so files and fail before tests start; either set include-hidden-files: true or stage these libraries under a non-hidden path.
Useful? React with 👍 / 👎.
Why
clice's existing tests speak LSP through pygls or replay recorded sessions, so nothing exercised a real editor end to end. The very first run of the new nvim harness caught the
initializecapability-decoding bug independently fixed on main by #455 — exactly the class of breakage this layer exists for: real clients, latest stable versions, full startup path.What
hello_world,modules/hover_on_imported_symbol}): startup/attach, didOpen + first diagnostics, index readiness, hover, definition (asserting the modules fixture jumps across the module boundary intodefs.cppm), completion, clean shutdown. Assertions stop at "well-formed, non-empty" — content correctness stays in the pytest suites. Test code lives with each editor client:editors/nvim/tests/e2e.lua(driven bynvim -l, starting clice through the shipped LSP config) andeditors/vscode/src/test/e2e.test.ts(via@vscode/test-cli;pretestnow compiles tests too —out/test/was never built before, so the existing sample suite never actually ran).test-editorjob installs the latest stable nvim + VSCode — deliberately unpinned, this job exists to catch new-editor-version breakage. Zed extension gets awasm32-wasip1compile check.pixi run -e editor editor-test, fixture prep intests/prepare.py; documented in the dev guide (docs/{en,zh}/dev/test-and-debug.md).docs/{en,zh}/guide/editors.mdwith generic LSP client snippets for Helix, Emacs, Sublime Text, Kate and Vim.Verification
All run locally (rebased on #455): unit 599 (Debug + RelWithDebInfo), integration 172, smoke 3/3, nvim E2E 2/2 (real nvim 0.12.3 stable), VSCode E2E 2×6 (real VSCode 1.124.2 under WSLg),
vsce package,pixi run format.Notes for review
testjobsneeds: build, which gates on all five native build legs (GitHub Actions cannot depend on single matrix legs); a one-leg build failure now skips all test jobs instead of just its own.test-editor) run in thetest-runpixi env relying on the runner's system toolchain for fixture CDB generation — same pattern the greentest-crossjobs already use, but windows-2025/ubuntu-24.04 x64 exercise it for the first time.Summary by CodeRabbit
New Features
Tests
Documentation