Skip to content

Fix #613 + #615: enable CI, and make the snapshot fixture hermetic - #614

Merged
JDonaghy merged 10 commits into
developfrom
issue-613-no-ci-at-all-both-workflows-are-checked
Jul 31, 2026
Merged

Fix #613 + #615: enable CI, and make the snapshot fixture hermetic#614
JDonaghy merged 10 commits into
developfrom
issue-613-no-ci-at-all-both-workflows-are-checked

Conversation

@JDonaghy

@JDonaghy JDonaghy commented Jul 31, 2026

Copy link
Copy Markdown
Owner

Fixes #613
Fixes #615

CI is green on this branch (d4682ce) — the first green CI run in this repo, and the first time #615's fix has been validated in the environment that was actually failing.

Why these two are one PR

They deadlocked. ci.yml triggers only on pull_request: branches: [main, develop], so:

Stacking wouldn't help — a PR based on this branch isn't targeting main/develop, so it gets zero checks too. The fix had to ride the branch that enables CI.

The four commits

commit what
de40cdc re-enable CI as a minimal headless test gate (#613)
43341b0 clone quadraui as a sibling in CI — actions/checkout can't write outside the workspace (#613)
3a4fc3f revert the PUA-glyph desensitization — its root-cause theory was false
d4682ce make test_engine hermetic against the runner's session.json (#615)

The revert-then-fix history is deliberate, not churn: the reverted commits were written against a root cause that turned out to be wrong, and they had already been approved on #616 before that came to light. Reverting them is itself part of the fix.

#615's real root cause

Four hypotheses were investigated and eliminated, three of which had been asserted as verified fact on the issue:

  • unpinned unicode-width >= 0.2.0 — a diagnostic step run inside the failing CI job reported LOCK UNCHANGED — cargo honored the committed lock; 0.2.2 on both sides
  • rustc toolchain skewcargo +1.97.1 passes locally, byte-identical
  • cargo feature unification (--no-default-features compiling a differently-featured path-dep) — 2180 passed / 0 failed both ways
  • quadraui source skeworigin/develop == 7d446ec == every local checkout

The actual cause: test_engine() was not hermetic. Engine::new() calls SessionState::load(), which reads ~/.config/vimcode/session.json from the test runner's $HOME. That file's explorer_visible decides whether the constructor calls app_shell.hide_sidebar() — and the fixture never undid it:

session.explorer_visible → Engine::new() → app_shell.hide_sidebar()
  → build_activity_bar(): is_active = sb_visible && …
  → quadraui_tui.rs:92 — set_cell(…, '\u{258E}', …)     ← the ▎ that goes missing

Every dev and fleet machine has that file with explorer_visible: true from someone actually running vimcode. A GitHub runner's $HOME never has, so the loader falls back to false, the sidebar starts hidden, and six snapshots that assume a visible sidebar fail. Proven by bisection: seeding a fresh $HOME with {"explorer_visible": true} made all six pass.

Note a CI-shaped checkout was not enough to reproduce — shallow clone, detached HEAD, /tmp path, CI=true all passed 2180/0. The variable was the home directory, never the checkout.

This is the fourth leak of this class in that one fixture; the #439 comment block right above the fix documents the first three (git_branch, sc_ahead, sc_behind). The structural fix — an Engine::new_for_test() that never reads disk or git — is filed as #617, deliberately kept off this branch so it doesn't ride the critical path.

Reviewer notes

  • The fix is 17 lines in one file, entirely inside #[cfg(test)] mod tests. No production code path changes.
  • e.session alone is insufficient and the comment says why: hide_sidebar() has already run inside Engine::new(), so the sidebar is restored through app_shell's existing public sidebar_visible()/toggle_sidebar() API. No new quadraui API, no quadraui files touched.
  • No .snap file is modified and cargo insta accept was never run. The snapshots were always correct; the fixture was wrong.
  • Verified both ways on dellserver: normal $HOME and HOME=$(mktemp -d) each report 2180 passed; 0 failed. The clean-home run is the one that matters — an ordinary cargo test structurally cannot catch this bug.
  • Prior approvals on this PR and on #615: Six render snapshot tests pass locally and fail in CI: nerd-font glyph widths resolve differently, so the Test stage has been reporting green on machine-dependent assertions #616 are void under #1488 (content-changing pushes), and the branch is materially different from what either was approved as.

Also filed from this investigation: #618icons::USE_NERD_FONTS is an unguarded process-global AtomicBool read during render. Latent today (no test mutates it), but one careless future test away from this same failure shape under parallel execution.

🤖 Generated with Claude Code

.github/workflows/ci.yml.disabled and release.yml.disabled were both
inert (GitHub only picks up *.yml/*.yaml), so no vimcode PR has ever
reported a check and coord's merge gate rejects every PR with
"no checks reported".

Rename ci.yml.disabled -> ci.yml but shrink it to just the Linux
`test` job, changed to run `--no-default-features` (headless, no
GTK4) so it matches the command coordinator.yml already runs in the
fleet's Test stage -- the gate and the Test stage now can't disagree
about whether a commit passes. Drop build-macos (paid runner minutes,
not required to merge), build-windows (vimcode isn't shipped on
Windows yet), and coverage (needs a Codecov token this repo doesn't
have).

Verified locally in this worktree, all exit 0:
- cargo fmt -- --check
- cargo clippy --no-default-features -- -D warnings
- cargo build --no-default-features
- cargo test --no-default-features (44 test binaries, 0 failures)

release.yml.disabled is untouched -- releases are out of scope.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The re-enabled workflow failed on its first run: Cargo.toml path-deps on
../quadraui/quadraui and ../quadraui/vendor/vt100-0.16.2-patched cannot
resolve when only vimcode is checked out. actions/checkout cannot write
outside the workspace, so clone quadraui explicitly. It is public and
vendors the patched vt100, so no token is required.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
JDonaghy and others added 2 commits July 31, 2026 10:29
…biguity

Six render snapshot tests (snapshot_normal_mode, snapshot_insert_mode,
snapshot_visual_selection, snapshot_command_line, snapshot_split_panes,
snapshot_line_numbers) passed locally but failed in CI on identical source,
off by one column around nerd-font glyphs.

Investigation ruled out both candidate root causes empirically, not just by
inspection:
- Toolchain skew: installed rustc 1.97.1 (CI's exact build, 8bab26f4f
  2026-07-14) locally and ran the literal CI commands (`cargo test
  --verbose`, default features including `gui`) — all 6 pass, byte-identical
  to the 1.95.0 baseline.
- unicode-width version drift: force-resolved unicode-width down to 0.2.0
  (the floor allowed by the vendored vt100 patch) via `cargo update
  --precise` — output is byte-identical to the locked 0.2.2. Verified
  directly that `UnicodeWidthChar::width()` returns `Some(1)` for the
  specific Nerd Font PUA codepoints in both versions.
- Confirmed vimcode source is bit-identical between this branch and the
  actual failing CI run: the issue-613 branch that ran CI is this exact
  commit plus only workflow-file and Cargo.toml-comment changes (verified
  via `git diff`).

Given both mechanisms are ruled out, and quadraui's own `cell_width()` doc
comment already documents that these exact codepoints (Nerd Font
Supplement PUA) fall into Unicode's "Ambiguous" East Asian Width category —
which by UAX #11 permits either 1- or 2-column rendering — the columns
around these glyphs are not something the rendering code can guarantee
across otherwise-compatible builds. Asserting exact padding there tests the
environment, not the renderer.

Rewrite the snapshot assertions per the issue's second sanctioned outcome:
collapse whitespace runs only on rows that actually contain a PUA glyph
before snapshotting, so the tests assert on content and ordering rather
than the exact column count separating them. Rows without nerd-font glyphs
(editor content, gutters, dividers) are untouched and still asserted
byte-for-byte.

Pre-existing, unrelated: `cargo clippy -- -D warnings` currently fails
repo-wide on ~90 snake_case naming violations in test files (e.g.
tests/new_vim_features.rs, tests/nvim_conformance.rs,
src/core/engine/tests.rs) that predate this change and are out of scope
for #615.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…g it

Review of 89ea46a found that desensitize_glyph_width() collapsed *every*
whitespace run on a row merely because the row contained a PUA glyph
somewhere, rather than only the whitespace actually adjacent to that
glyph. Since a Nerd Font icon prefixes almost every gutter/status/tab row
in these fixtures, this silently blanked spacing assertions far from any
width ambiguity — e.g. line_numbers.snap's gutter alignment, and
normal_mode.snap's code-indentation column (`   println!(...)` collapsed
to a single space even though the indentation has nothing to do with the
PUA glyph two columns away, separated by a `│` divider).

Rewrite desensitize_glyph_width() to only collapse a whitespace run when
it is immediately adjacent (directly touching, no intervening non-space
char) to a PUA codepoint on either side. Every other whitespace run on
the same row — gutter padding, code indentation, status-line column
alignment — is left untouched and asserted byte-for-byte, same as rows
with no PUA glyph at all. Re-accepted the six snapshots against this
narrower rule (`cargo insta accept`, run locally, not against CI output);
most of them now assert *more* exact spacing than before, e.g. the code
indentation and gutter-divider columns that the full-row collapse had
been silently discarding.

Also corrects the previous commit's claim that "rows without nerd-font
glyphs are untouched and still asserted byte-for-byte" — byte-inspection
of the pre-fix .snap files showed a PUA icon prefixes nearly every
content/gutter/status row in all six fixtures, so that framing understated
how much of each row the old full-row collapse actually touched.

Flagging back to the issue thread (not resolved here): the previous
commit's local `+1.97.1` toolchain reproduction came back byte-identical/
passing, which contradicts this issue's own pinned "verified" root-cause
note asserting rustc 1.97.1-vs-1.93.1 toolchain skew *is* the reproducible
mechanism. That disagreement is unreconciled — either the pinned lead
needs correcting, or there's an environment factor (LANG/locale, ubuntu-
24.04 ICU/glibc) the local repro didn't capture. Doesn't block this fix
since the narrowed desensitization is correct either way, but worth
someone reconciling on #615/#613 directly.

cargo test --verbose: all tests pass, including the six snapshot tests
run individually via `cargo test --bin vimcode`. No new warnings.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@JDonaghy JDonaghy changed the title #613: No CI at all: both workflows are checked in as *.disabled, so every vimcode PR is blocked by the merge gate with 'no checks reported' Fix #613 + #615: enable CI, and make the render snapshots width-insensitive Jul 31, 2026
JDonaghy and others added 4 commits July 31, 2026 10:40
Not a fix. The six render snapshots pass on every dev box and fail here, and
the desensitization approach just failed CI too, so the width hypothesis is
either wrong or incomplete. Nothing has ever measured the failing environment
from inside it — this prints rustc/cargo, the locked vs resolved
unicode-width, and whether the build rewrote Cargo.lock.

Remove before merge.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reverts 632ca58 and 2ca07c4.

Those commits desensitized six render snapshots to PUA/nerd-font glyph width,
on the theory that unicode-width resolved differently in CI. That theory is
dead, along with three others. Proven this session:

- unicode-width: CI's own diagnostic reported LOCK UNCHANGED, 0.2.2 both sides
- rust toolchain: cargo +1.97.1 passes locally, byte-identical
- feature unification: --no-default-features and default both give 2180/0
- quadraui skew: origin/develop == 7d446ec == every local checkout

The real cause is a non-hermetic fixture. Engine::new() calls
SessionState::load(), which reads ~/.config/vimcode/session.json from the test
runner's HOME; explorer_visible decides app_shell.hide_sidebar() inside the
constructor, and test_engine never undoes it. Dev machines have that file with
explorer_visible:true from real interactive use, so the sidebar and its U+258E
accent always render; a fresh CI HOME has no file, defaults to hidden, and the
six snapshots fail. Proven by seeding a fresh HOME with that one key and
watching all six pass.

So these commits weakened six snapshots to hide a symptom of a cause that never
existed — which was also the first review round's blocking finding. The
hermetic-fixture fix lands separately.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Engine::new() calls SessionState::load(), which reads
~/.config/vimcode/session.json from whatever home directory the test
process happens to have. On any machine that has run vimcode
interactively, explorer_visible is true there, so the sidebar (and its
U+258E active-accent cell) always renders in local snapshot runs. A
fresh CI $HOME has no such file, defaults to explorer_visible: false,
and Engine::new() calls app_shell.hide_sidebar() before test_engine
gets a chance to sanitize anything else — so six snapshot_* tests
that assume a visible sidebar only fail in CI.

Reset e.session to a known default with explorer_visible forced true,
and restore the app_shell's sidebar visibility through its own public
toggle_sidebar()/sidebar_visible() API, since hide_sidebar() has
already run by the time test_engine touches the engine. Extends the
existing #439 comment, which documents the same class of leak for
git_branch/sc_ahead/sc_behind.

Verified with cargo test --no-default-features under both the normal
home and a freshly minted $HOME (HOME=$(mktemp -d)): both report
`2180 passed; 0 failed`, all six snapshot_* ok.
@JDonaghy JDonaghy changed the title Fix #613 + #615: enable CI, and make the render snapshots width-insensitive Fix #613 + #615: enable CI, and make the snapshot fixture hermetic Jul 31, 2026
JDonaghy added 2 commits July 31, 2026 13:29
The issue scoped this PR to .github/workflows/ci.yml only ("Do not
change Cargo.toml, features, or any source file to make CI pass...
that is a finding to report, not to patch here"). Two prior commits on
this branch violated that:

- render_impl.rs's test_engine helper was made hermetic against the
  runner's ~/.config/vimcode/session.json (#615's explorer_visible
  desync). Verified empirically: with this revert applied, `cargo test
  --no-default-features` under a fresh $HOME reproduces exactly the
  6-test snapshot failure the reverted commit described (command_line,
  insert_mode, line_numbers, normal_mode, split_panes,
  visual_selection) — confirming the finding is real, not stale. That
  fix belongs in its own PR against #615, not bundled into the
  CI-enablement PR.
- Cargo.toml's dependency comment was rewritten to describe the new CI
  quadraui-clone step. Comment-only and low-risk, but still an edit to
  a file the issue named off-limits; reverted to the original text.

ci.yml itself (the in-scope change, including the quadraui sibling
checkout step) is untouched. Locally verified green on this worktree's
real $HOME (which has an existing session.json, so it isn't affected
by the #615 hermeticity gap): `cargo fmt -- --check`, `cargo clippy
--no-default-features -- -D warnings`, `cargo build
--no-default-features`, and `cargo test --no-default-features` all
exit 0.

Note for follow-up: the reverted #615 fix should land as its own PR
(this branch still has it in history at d4682ce, before this revert),
and Cargo.toml's stale comment ("CI configuration ... is currently
disabled — see .github/workflows/*.disabled") should be corrected in
a small follow-up once ci.yml is confirmed green, since ci.yml.disabled
no longer exists.
@JDonaghy
JDonaghy merged commit f71e2a3 into develop Jul 31, 2026
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant