Repository navigation
ci(windows): target the Windows leg at what actually broke on Windows - #640
Conversation
Three issues this reporting session (#633, #634, #636) were all filed by a Windows end user and all slipped past ci.yml. A test-windows job already exists (runs-on: windows-latest, added alongside issue #621's fix): it runs a curated, hand-grown list of eight test files plus a real bash scripts/install-plugin.sh exercise of the install path. The list grew reactively, one file per Windows bug fixed, and never included tests_py/handlers/consolidation/, where issue #636 lived -- so nothing #636 touched has ever run on Windows in CI. A separate real gap: the job installs dependencies via pip install -r requirements/ci-sqlite-min.txt, a hash-pinned shortcut that always carries the [sqlite] extra. The real install path (install-plugin.sh -> scripts/setup.py -> ensure_deps() reading launcher_pins.py::BASE_PACKAGES) never named sqlite-vec there -- issue #634's exact root cause. The job already runs that real path via install-plugin.sh, but nothing checked its output, so the one dependency gap Windows CI could have caught directly went unchecked regardless. Two additive changes, nothing else in the job's shape or budget touched: Added the four existing test files issues #634 and #636 actually changed to the curated list, following the exact convention already in place ("the new file joins the Windows CI leg beside its sibling"): tests_py/infrastructure/test_semantic_fallback_169.py and tests_py/test_doctor.py (#634's sqlite-vec fix), tests_py/handlers/consolidation/test_wiki_maintenance.py and tests_py/handlers/consolidation/test_wiki_backlog_pass.py (#636's fix). tests_py/scripts/test_install_plugin_marker_order.py (#633's fix) is deliberately not added here: it only exists on the still-open fix/issue-633-backend-marker branch, and referencing a nonexistent path would break this job for every unrelated PR until that branch merges. That branch's own PR should add it, matching the same convention. Added one new step, scripts/verify_sqlite_extra_parity.py, run immediately after install-plugin.sh. It parses pyproject.toml's [sqlite] extra directly and checks each declared package resolves as an installed distribution under the real install path's own deps/ directory (importlib.metadata.Distribution.discover, PEP 503 name normalization) -- the direct, minimal check that would have caught #634, since it asserts against the same path a real user's machine goes through, not a CI-only shortcut. An earlier version of this change replaced the curated list with the full suite, doubled the job's timeout with no measured basis, and restructured several steps. Reviewed and rejected: the curated, additive, one-file-per-fix pattern is this job's established convention, not an oversight to correct wholesale, and an unmeasured timeout change is exactly the kind of unsupported figure this repo's own standard forbids. This commit is the targeted rework. ADR-1091 records the decision. Full local pytest (tests_py/scripts/): 1091 passed, 3 skipped. actionlint and a YAML parse both clean on the modified workflow. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01MyRfKj1vpBm2yx3sXCN8ik
PR #640's own Windows CI run caught the bug this commit fixes: Test (Python 3.10) failed at collection with ModuleNotFoundError: No module named 'tomllib'. tomllib is 3.11+ stdlib; this repo's floor is 3.10 (pyproject.toml requires-python), and every ubuntu Test (Python 3.x) matrix leg collects every file under tests_py/, including this PR's own new test file, regardless of which Python version the test itself targets. Replaced tomllib.loads() with a scoped regex extracting the `sqlite = [...]` array's quoted strings directly: pyproject.toml's [sqlite] extra is a short, single-key array, so a general TOML parser was more machinery than the actual parsing need, and this avoids either a version-gated stdlib import or adding a third-party TOML dependency for one list. sqlite_extra_names()'s return value is unchanged for every existing input; all 12 tests in test_verify_sqlite_extra_parity.py still pass against the rewritten implementation. The same CI run's Windows job independently confirmed the new verifier step works as designed: it reported sqlite-vec missing under the real install path's deps/ output, exactly issue #634's defect, live on Windows, on main as it stands today (PR #637 not yet merged). That failure is the check working correctly, not a bug in this PR -- noted here since the two failures landed in the same CI run and only one of them needed a code fix. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01MyRfKj1vpBm2yx3sXCN8ik
|
ZETETIC-REVIEW: APPROVE Stakes: High (ci.yml has 4+ distinct authors in the last 90 days, and this touches the release-gating Windows job). Full review moves applied. Move 0 (ledger/seen-defect check): the PR body's "Known dependency on other open PRs" section discloses the current Windows job failure with a reason and a cited dependent PR (#637), rather than waving it off as unrelated. Verified directly: read the actual CI log for the latest run (job 107114564857). The verifier fails with exit code 1 and the exact message "Missing under D:\a_temp\cortex-plugin-data\deps: sqlite-vec ... issue #634's exact fix", the check working as designed, not a defect being swept aside. No un-issued rationalization found. CI diff scope: confirmed minimal and additive. gh pr diff 640 shows exactly one new step (verify_sqlite_extra_parity.py, run right after install-plugin.sh) and four existing test files appended to the job's curated list. No timeout changes, no step reordering, no restructuring. Matches the owner's stated constraint and ADR-1091's own account of the rejected first version. Path correctness: scripts/setup.py defines DEPS_DIR = os.path.join(PLUGIN_DATA, "deps"), which matches the $CLAUDE_PLUGIN_DATA/deps argument the new CI step passes to the verifier. This confirms the check is not pointed at the wrong directory and will not fail forever after #637 merges. Also confirmed PR #637 does add sqlite-vec to launcher_pins.py::BASE_PACKAGES and requirements/setup.txt, backing the "goes green once #637 merges" claim. All four newly-referenced test files exist on this branch at the paths cited. Regex scoping: pyproject.toml has exactly one "sqlite = [" occurrence (line 93, under [project.optional-dependencies]), so the scoped, non-tomllib regex cannot misparse an unrelated block. The tomllib-avoidance rationale (3.10 floor, tomllib is 3.11+) is accurate and documented inline. Fail-loud behavior: main() returns 1 and writes the missing-package list plus a remediation pointer to stderr on failure, 0 and an OK message on success. No warn-and-continue path exists. Confirmed by both the source and the live CI failure log. Craftsmanship: script is 112 lines, no function over 40, PYPROJECT_TOML path stays a module constant with an inline source comment, ADR-1091 backs the change. Test file mirrors an existing sibling's structure and covers the parser, PEP 503 normalization, missing-deps-dir handling, and the CLI exit codes. One non-blocking correction to the PR's own test-plan claim: the mutant described as "dropped deps_dir.is_dir() check, caught by test_nonexistent_deps_dir_reports_every_name_missing" is actually equivalent, not killed. importlib.metadata.Distribution.discover on a nonexistent path already returns nothing regardless of the guard, so removing the guard does not change observable behavior for that test. Worth a doc fix, not a blocker; the guard itself is still reasonable defensive code and the rest of the mutation-survival claims in the PR body check out. CHANGELOG.md is unchecked in the reviewer checklist. Acceptable here since this repo's convention, confirmed against recent entries, is to batch CHANGELOG updates at release cut rather than per merged PR, and this PR's own checklist honestly leaves it unchecked instead of claiming compliance it does not have. |
…uite # Conflicts: # wiki/manifest.json
CI caught this on Windows once PR #640 added tests_py/test_doctor.py to the curated Windows CI test list: test_pg_driver_passes_in_dev_env asserts _pg_driver().ok is True on the bare assumption "dev env has postgresql extras installed", which doesn't hold on the Windows job's requirements/ci-sqlite-min.txt (deliberately psycopg-free) -- the same assumption is false even in this repo's own local dev venv (psycopg is not importable here either), so this was never actually a dev-only edge case, just never previously exercised. mcp_server/doctor.py's own SQLITE_CHECKS explicitly excludes _pg_driver already, confirming it is not a SQLite-relevant check. Marked with tests_py/conftest.py's existing requires_psycopg marker, the same pattern already used by several sibling test files for exactly this class of dependency. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01MyRfKj1vpBm2yx3sXCN8ik
|
ZETETIC-REVIEW: APPROVE Re-verification at head e55eb02, superseding the prior verdict (stale: it predated a required merge of origin/main after #637/#638/#639 all landed first, plus one genuine follow-up fix). Confirmed via One genuine follow-up: once #637 landed sqlite-vec in BASE_PACKAGES, the Windows job's real failure (the thing this PR's verifier is designed to catch) went away, and CI then caught a second, unrelated pre-existing gap: adding tests_py/test_doctor.py to the curated Windows list (this PR's own change) pulled in test_pg_driver_passes_in_dev_env, which assumes psycopg is importable -- false on the Windows job's deliberately psycopg-free requirements/ci-sqlite-min.txt, and also false in this repo's own local dev venv, so not actually a Windows-only edge case. Fixed with tests_py/conftest.py's existing CI is fully green on e55eb02, including the Windows job for the first time. |
Summary
Issues #633, #634, and #636 this reporting session were all filed by a Windows end user and all slipped past
ci.yml. Atest-windowsjob already exists (runs-on: windows-latest), running a curated, hand-grown list of eight test files plus a realbash scripts/install-plugin.shexercise of the install path. The list grew reactively, one file per Windows bug fixed, and never includedtests_py/handlers/consolidation/, where issue #636 lived, so nothing #636 touched has ever run on Windows in CI.A separate real gap: the job installs dependencies via
pip install -r requirements/ci-sqlite-min.txt, a hash-pinned shortcut that always carries the[sqlite]extra. The real install path (install-plugin.shtoscripts/setup.pytoensure_deps()readinglauncher_pins.py::BASE_PACKAGES) never namedsqlite-vecthere, which is issue #634's exact root cause. The job already runs that real path viainstall-plugin.sh, but nothing checked its output, so the one dependency gap Windows CI could have caught directly went unchecked regardless.Two additive changes, nothing else in the job's shape or budget touched. Added the four existing test files issues #634 and #636 actually changed to the curated list, following the exact convention already in place ("the new file joins the Windows CI leg beside its sibling"). Added one new step,
scripts/verify_sqlite_extra_parity.py, run immediately afterinstall-plugin.sh, checking every packagepyproject.toml's[sqlite]extra declares actually resolves under the real install path's owndeps/output.An earlier version of this change replaced the curated list with the full suite, doubled the job's timeout with no measured basis, and restructured several steps. Reviewed and rejected in this session: the curated, additive, one-file-per-fix pattern is this job's established convention, not an oversight to correct wholesale, and an unmeasured timeout change is exactly the kind of unsupported figure this repo's own standard forbids. This PR is the targeted rework.
Closes nothing directly; supports #633, #634, #636.
Type of change
Test plan
tests_py/scripts/full run, 1091 passed, 3 skipped, on this machine (darwin; the point of this PR is to also run on Windows CI itself).tests_py/scripts/test_verify_sqlite_extra_parity.py, 12 tests covering the TOML parser, the deps-dir comparison (including PEP 503 hyphen/underscore normalization against real wheel METADATA shape), and the CLI.missing_from_deps_diris caught bytest_normalizes_hyphen_underscore_like_pep_503; a mutant that stopped checkingdeps_dir.is_dir()is caught bytest_nonexistent_deps_dir_reports_every_name_missing.actionlint .github/workflows/ci.ymlclean;python -c "import yaml; yaml.safe_load(...)"parses clean.Audit notes
tests_py/scripts/test_install_plugin_marker_order.py(issue Upgrade 3.x → 4.x leaves the plugin on PostgreSQL: backend.json is never written when setup.py fails #633's own new regression test) is deliberately not added to the Windows list here: it only exists on the still-openfix/issue-633-backend-markerbranch (PR fix(setup): persist backend marker before setup.py can fail; close ML-child isolation gap #638), and referencing a nonexistent path would break this job for every unrelated PR until that branch merges. That branch's own PR should add it, matching the same convention, once it lands.Coding-standards compliance
any/ untyped dicts at boundaries# source: ADR-1091pointersBreaking changes
None.
Reviewer checklist
Known dependency on other open PRs
The new
verify_sqlite_extra_parity.pystep will report a real failure (missingsqlite-vecunder the real install path) until PR #637 (issue #634's fix) merges, sincemaindoes not yet have that fix. That is the check working correctly, not a false positive: once #637 merges, this Windows job goes green on that step.🤖 Generated with Claude Code