Skip to content

fix(launcher,wiki): install without a pip module, refuse an empty refine (#582, #583) - #584

Merged
cdeust merged 1 commit into
mainfrom
fix/launcher-pip-and-refine-contract
Sep 16, 2026
Merged

fix(launcher,wiki): install without a pip module, refuse an empty refine (#582, #583)#584
cdeust merged 1 commit into
mainfrom
fix/launcher-pip-and-refine-contract

Conversation

@cdeust

@cdeust cdeust commented Sep 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

Summary

Two fixes the review of #581 and the local suite surfaced.

scripts/launcher_pip.py built every install command as sys.executable -m pip, so an interpreter without a pip module could not fill the plugin's private deps/ at all, reporting No module named pip. A virtual environment made by uv venv or python -m venv --without-pip has no pip module by design. pip_entry() now resolves the installed module first, then the pip wheel CPython bundles for ensurepip, run in place from the wheel path so nothing is installed into the interpreter, and otherwise raises a FileNotFoundError naming that interpreter, which travels the OSError path both call sites already report (ADR-1067). tests_py/scripts/test_installer_upgrade_single_version.py seeds its fixture through the same resolution instead of hardcoding -m pip.

mcp_server/handlers/wiki_refine.py accepted a wiki_refine_draft call carrying nothing but a draft_id: synth_model always defaulted, so the draft was stamped with a model that wrote none of its content and a refined_llm memo recorded a refinement that never happened. Such a call is now refused whole, with no write to the draft and no memo (ADR-1068).

Closes #582
Closes #583

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Test plan

  • New tests_py/scripts/test_launcher_pip_entry.py pins the resolution order: installed module preferred; bundled wheel used when no pip module is importable; that resolved command answering as pip when actually run; the interpreter named when neither exists; _pip_command built on the resolved entry. Four of the five fail on the previous code.
  • New test_a_refine_carrying_no_content_writes_nothing in tests_py/handlers/test_wiki_refine_confidence.py asserts the draft's synth_model and lead are unchanged, the memo count for that draft is unchanged, and the response is an error. It fails on the previous handler.
  • The 12 tests of test_installer_rerun_keeps_entries.py and test_installer_upgrade_single_version.py pass in this pip-less uv venv, where 11 of them errored in setup before.
  • Full suite: 8675 passed, 8 skipped, no errors.
  • ruff check, ruff format --check: clean. pyright mcp_server/: 0 errors. scripts/check_craftsmanship.py --base origin/main: OK. scripts/check_doc_claims.py: OK.

Audit notes

  • bundled_pip_wheel() locates ensurepip with importlib.util.find_spec rather than importing it, so a standard library without the module returns None instead of raising at import time.
  • The bundled wheel's pip can be older than an installed pip. Installs are pinned and hash-checked (ADR-1059), so the resolver version selects no different distribution.

Coding-standards compliance

  • §2.2 Layer dependency direction preserved.
  • §4.1 / §4.2 No new size violation; the craftsmanship gate passes against origin/main.
  • §8 No numeric constant added.
  • §9 No dead code, no TODO.

Breaking changes

None. A wiki_refine_draft call with only a draft_id used to return updated: true after writing a memo; it now returns an error. The tool has been registered only since #581 and the store holds one such memo, from a test runner in April 2026.

Reviewer checklist

  • CHANGELOG.md updated under Unreleased / Fixed.
  • Documentation updated (ADR-1067, ADR-1068, the wiki_refine_draft tool description).
  • No secrets or PII in the diff.
  • CI passes on the latest commit.

@cdeust @claude
fix(launcher,wiki): install without a pip module, refuse an empty ref…
54fe626 
…ine (#582, #583)

The launcher built every install command as `sys.executable -m pip`, so
from an interpreter without a pip module (a `uv venv`, or `python -m venv
--without-pip`) no dependency could be installed at all.
`launcher_pip.pip_entry()` now resolves the installed module first, then
the pip wheel the standard library bundles for ensurepip, run in place so
nothing is installed into the interpreter, and otherwise raises an error
naming the interpreter (ADR-1067). Measured in a `uv venv` on Python
3.13.7: the 12 installer tests pass where 11 errored in setup.

`wiki_refine_draft` accepted a call carrying nothing but a draft_id: it
stamped the draft's synth_model and wrote a refined_llm memo recording a
refinement that never happened. Such a call is now refused whole, with no
write to the draft and no memo (ADR-1068). The behaviour predates #579;
registering the tool is what made it reachable.

Full suite: 8675 passed, 8 skipped.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01Cz7A6goxVzfKL9NT7k4zDH
@cdeust

cdeust commented Sep 16, 2026

Copy link
Copy Markdown
Owner Author

ZETETIC-REVIEW: APPROVE

Summary

Both fixes are correct, mutation-tested, and match the PR's claims exactly. launcher_pip.pip_entry() resolves installed-pip → bundled-ensurepip-wheel → named FileNotFoundError, and both call sites (launcher_deps_install.py::pip_install, install_requirements) genuinely catch it via their existing except OSError handlers (FileNotFoundError is an OSError subclass) - no traceback, clean False return. wiki_refine.py's _CONTENT_FIELDS refusal is correct for every shape I drove it against, including the empty-string/empty-list/empty-dict edge cases the task asked about. One CI job (Test (SQLite backend)) was cancelled on a 10-minute execution-time budget; evidence below indicates this is a general runner-slowness/timeout-budget issue this run, not a defect in the diff - flagged as a merge-blocking item to resolve with a rerun, separate from the code-review verdict.

Move 0 - Ledger reconciliation and seen-defect check (§13.2 + §14)

  • No embedded Completion Ledger in the PR body; independently re-enumerated the diff's paths myself (11 files: CHANGELOG, 2 ADR pairs (docs/ + wiki mirror), handler, launcher_pip.py, 3 test files, wiki/manifest.json). All accounted for; no unmapped branch/error-arm.
  • Seen-defect rationalizations: none found in the diff or PR body. The reviewer checklist explicitly leaves "CI passes on latest commit" unchecked - the author did not claim green CI, so there is nothing here to refuse over.
  • Verdict short-circuit: N/A, proceeding.

Stakes calibration (Move 7)

  • Classification: Medium.
  • Criterion: core packaging/installer logic used by every plugin install, plus a documented breaking change to a registered MCP tool contract (wiki_refine_draft); not auth/billing/crypto/migrations, not >400 lines, <2 distinct authors in 90d (cdeust/Clement are the same person per git log).
  • Depth applied: full Moves 1-6.

Layer check (Move 1)

File Layer Imports added/changed Verdict
scripts/launcher_pip.py scripts (stdlib-only tooling) +import importlib.util (stdlib) OK - no layer crossed
mcp_server/handlers/wiki_refine.py handlers none added; existing infrastructure.* imports only OK

SOLID audit (Move 2)

No violations. pip_entry()/bundled_pip_wheel() are new, single-purpose functions (resolution only); _pip_command was refactored to depend on the new entry point rather than growing a branch (this is the OCP-friendly shape - no new if backend == X added to the install path). The refusal in handler_refine is a guard clause, not a type-switch.

Wiring & contract drift (Move 3)

  • New public symbols: launcher_pip.bundled_pip_wheel(), launcher_pip.pip_entry(). Both wired: pip_entry() is called from _pip_command() (still the sole caller of _install/resolve/install_requirements), and independently by tests_py/scripts/test_installer_upgrade_single_version.py::_pip_entry().
  • _CONTENT_FIELDS constant: wired into the new refusal branch in handler_refine; also referenced by the updated tool description string (schema_refine["description"]) - consistent.
  • Contract change: wiki_refine_draft now refuses a draft_id-only call. PR body documents this as the only breaking change and cites the one historical caller (a test-runner memo, April 2026) - acceptable, no other callers in-repo (grep -rl "wiki_refine_draft" confirms no internal caller besides the handler/registry/tests).
  • No dead code, no bare TODOs, no debug prints in the diff.

Test adequacy (Move 4) - evidence, not assertion

Ran everything myself in the pip-less worktree venv (.venv/bin/python3, confirmed ModuleNotFoundError: No module named 'pip', confirmed ensurepip bundles pip-25.0.1-py3-none-any.whl under uv's cpython-3.12.11):

  1. pip_entry() resolution order, driven directly:
    pip_entry() → ['.../pip-refine/.venv/bin/python3', '.../ensurepip/_bundled/pip-25.0.1-py3-none-any.whl/pip'] (bundled-wheel branch, correct - no installed module).

  2. Failure reaches existing reporting, no traceback. With both find_spec("pip") and bundled_pip_wheel() monkeypatched to fail-both:

    • launcher_deps_install.pip_install(...) → prints [cortex-launcher] dependency resolution failed: ... has no pip module and its standard library bundles no pip wheel for ensurepip... and returns False. Confirmed via except (OSError, _cpu.CpuWheelError) at scripts/launcher_deps_install.py:230.
    • launcher_deps_install.install_requirements(...) → same pattern via except OSError at scripts/launcher_deps_install.py:253. FileNotFoundError is an OSError subclass (stdlib), confirmed both catch it - no crash, no bare traceback.

  3. Real install, not just --version. Built a wheel for a synthetic probepkg==1.0, wrote a hash-pinned requirements.txt with --no-index --find-links <wheel dir> (pip honors these as in-file directives, same trick test_installer_upgrade_single_version.py doesn't need since it drives _pip_target directly - I drove the higher-level scripts/launcher_deps.py --requirement <file> <deps_dir> entry point instead, per the task's ask), then ran it in the pip-less venv:
    UV_NO_SYNC=1 uv run python scripts/launcher_deps.py --requirement requirements.txt depsout → exit 0, depsout/probepkg/__init__.py, depsout/probepkg-1.0.dist-info/{RECORD,WHEEL,METADATA,INSTALLER,REQUESTED} all present. Genuine install via the bundled-wheel pip, in the pip-less interpreter.

  4. bundled_pip_wheel() correctness:

    • ensurepip present, _bundled non-empty (the real case here): returns the sorted-last pip-*.whl - correct, and sorted() + [-1] picks the highest version string if ever more than one wheel is bundled (CPython historically ships exactly one, but the code degrades gracefully if that ever changes).
    • ensurepip present but _bundled empty/missing: wheels is [], returns None - correct, falls through to the named error. (Not independently reproducible without patching a real stdlib, but the code path is a direct, exhaustively-tested glob-empty case identical to what test_an_interpreter_without_either_is_named_in_the_error monkeypatches.)
    • ensurepip absent (find_spec returns None, e.g. Debian python3-minimal): returns None immediately via the spec is None guard - correct.

  5. Suite numbers match exactly. UV_NO_SYNC=1 uv run pytest -q in this worktree: 8675 passed, 8 skipped ... in 278.41s - identical to the PR body's claim.

  6. _CONTENT_FIELDS refusal - every shape driven directly against a real SQLite-backed draft (via the existing sqlite_store fixture):

    • {"draft_id": N} (all four absent) → refused, updated: False, error names the four fields, draft/memo unchanged (this is the PR's own new test, reran and confirmed green).
    • {"draft_id": N, "lead": ""} → treated as content: updated: True, lead written as '', a refined_llm memo IS inserted (memo count 1→2). This is correct per the code's own semantics: args.get("lead") is "", not None, so it is not "absent" - and update_draft's own skip-on-None logic uses the identical predicate, so a client that explicitly sends lead="" is asking to clear the lead, a real edit, and the audit memo correctly records that a (trivial) edit happened. The MCP schema types lead as "string" with no null variant, so a client cannot send an explicit null to mean "omitted" - omission is the only way to signal "no opinion," and args.get() correctly returns None only for that case.
    • {"draft_id": N, "sections": []} → not refused by _CONTENT_FIELDS (an empty list is not None), but caught immediately after by the existing _validate_against_contract check (required section missing: 'Context', etc. for an ADR-kind draft) - so it still can't silently "succeed while doing nothing"; it fails validation instead. No write occurs either way.
    • {"draft_id": N, "frontmatter": {}} → same as lead="": treated as content, updated: True, memo written. Symmetric and consistent with update_draft's skip-on-None.
    • Net: the is None test is correct for ADR-1068's stated scope ("all absent") and does not misclassify any of the three edge shapes the task raised - they are all legitimate explicit edits, not silently-nothing refinements. Non-blocking gap: none of these three edge shapes has a test in this PR (only the all-None case is tested); worth a follow-up test for lead=""/frontmatter={} since the "counts as content" behavior is a real design decision someone could accidentally invert later.

Complexity & structure (Move 5)

pip_entry() (16 lines), bundled_pip_wheel() (11 lines), the refusal block in handler_refine (8 lines) - all well under the 40-line/300-line thresholds. scripts/check_craftsmanship.py --base origin/main → OK. No over-engineering smells.

Security & hygiene (Move 6)

  • No new I/O on user-controlled input beyond what already existed (pip install from a hash-pinned requirements file, unchanged trust model per ADR-1059).
  • bundled_pip_wheel() uses find_spec (no import side effect) - correctly avoids importing ensurepip (which has installation side effects) just to check for its presence.
  • Commit hygiene: single conventional commit (fix(launcher,wiki): ...), scoped to the two linked issues, no unrelated changes, no secrets.

Gates run (this worktree, .venv synced without pip per the task setup)

  • UV_NO_SYNC=1 uv run ruff check → All checks passed.
  • UV_NO_SYNC=1 uv run ruff format --check → 1458 files already formatted.
  • UV_NO_SYNC=1 uv run python scripts/check_craftsmanship.py --base origin/main → OK.
  • UV_NO_SYNC=1 uv run python scripts/check_doc_claims.py → OK (1 declared exemption, pre-existing, CONTRIBUTING.md:36).
  • .venv/bin/python -m pyright mcp_server/ → 0 errors, 0 warnings, 0 informations.
  • UV_NO_SYNC=1 uv run pytest -q → 8675 passed, 8 skipped, 459 subtests passed in 278.41s - matches the PR body exactly.
  • git status --short after all of the above → clean.

Mutation testing (Move 4 strength check, §12) - scratch copies, worktree untouched

Per the task's "scratch copy" instruction, mutated file copies under the scratchpad (never the worktree itself - git status --short stayed clean throughout, confirmed after every run):

  1. Dropped the bundled-wheel branch from pip_entry() (falls straight to the FileNotFoundError when no installed module): loaded the mutant as sys.modules["launcher_pip"] and ran tests_py/scripts/test_launcher_pip_entry.py → 1 failed (test_without_a_pip_module_the_bundled_wheel_is_used), 4 passed. Mutant killed.
  2. Changed the refusal condition if all(...) to if False in wiki_refine.py: loaded the mutant as sys.modules["mcp_server.handlers.wiki_refine"] via a pytest_configure plugin hook (needed so the test-isolation conftest guard runs first) and ran tests_py/handlers/test_wiki_refine_confidence.py → 1 failed (test_a_refine_carrying_no_content_writes_nothing, assert True is False), 3 passed. Mutant killed.
    Both fixes are mutation-strong, not just line-covered. git status --short in the worktree: clean before, during, and after.

Doc/ADR/CHANGELOG accuracy

Issues

Blocking (merge-gating, not code-defect)

  • CI job Test (SQLite backend) was cancelled (exceeded the 10-minute job budget; gh api repos/cdeust/Cortex/actions/jobs/104687037941 → conclusion: cancelled, annotation "The job has exceeded the maximum execution time of 10m0s"). Its own pytest step took 505.57s (8373 passed, 141 skipped) vs. 224.74s on the latest main run (8367 passed, 141 skipped) for the same suite minus 6 tests - a 2.25x slowdown I cannot attribute to this diff: Test (Python 3.10) (7m31s) and Test (Python 3.12) (8m49s) on this same PR run were also markedly slower than their typical ~5-6 min baseline while still completing, which points to general runner contention on this specific workflow run rather than a hang or regression introduced by the 6 new tests (each of which is sub-second in isolation, confirmed locally). Per this project's own rule (CI green + posted review = merge), do not merge on this run's red check - rerun CI and confirm Test (SQLite backend) completes within budget before merging. If the slowdown reproduces on a clean rerun, that would warrant reopening this as a real defect; on the evidence gathered here it looks like transient infra noise, not a defect in the diff.

Non-blocking

  • tests_py/handlers/test_wiki_refine_confidence.py - no test exercises lead="" or frontmatter={} as content (both correctly updated: True per the code's design, verified manually above); a follow-up test pinning that "explicit-empty counts as content" behavior would protect against someone later "fixing" _CONTENT_FIELDS's falsy-vs-None distinction by accident.

Hand-offs

None. No root-cause-fix, structural-decomposition, formal-correctness, cargo-cult, threat-modeling, or performance-claim concerns beyond this review's scope.

Verdict

APPROVE at the code-review level - both fixes are correct, minimal, mutation-tested, and match every claim in the PR body and ADRs that I could independently verify. Do not merge on the current CI run: rerun and confirm Test (SQLite backend) passes within its time budget first (see Blocking item above), per this project's CI-is-the-authority merge gate.

@cdeust
cdeust merged commit b71493b into main Sep 16, 2026
44 of 46 checks passed
@cdeust
cdeust deleted the fix/launcher-pip-and-refine-contract branch September 16, 2026 06:37
This was referenced Sep 16, 2026
Closed
Merged
Closed
Merged
cdeust added a commit that referenced this pull request Sep 16, 2026
@cdeust @claude
ci(test-sqlite): set the job budget from its measured p90 (#594)
120a5ea 
The 10-minute budget left 1.7x headroom over this job's p90, and two runs
of 2026-09-16 exceeded it: main at a981f93, cancelled at 616s having
reached 74% of the suite, which made CI Green fail on
test-sqlite='cancelled'; and PR #584's first attempt at 610s. Both passed
on a rerun, in 416s and 276s, with no code change. The main run's own
tree had taken 266s as the pull request of the commit it squashed.

Measured over that day's 21 job executions, every attempt counted: 18
successes from 234s to 416s, median 328s, p90 348s, and three
non-successes: the two timeouts above and a 47s cancellation from a
superseded push. The budget is now 20 minutes, 3.4x that p90, about twice
the point where both timeouts fired, and one eighteenth of GitHub's own
ceiling, with the measurement recorded beside the number so suite growth
into the new headroom stays visible (ADR-1074). The Windows and matrix
budgets are untouched: nothing measured here applies to them.

An earlier draft of this commit, its ADR and its CHANGELOG entry cited 29
runs, 27 successes and a second cancellation from a superseded push. That
sample crossed several days and counted only each run's latest attempt,
so it hid the two reruns and invented the 150s cancellation. The review
of PR #595 caught it; the numbers above are the reproducible ones.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01Cz7A6goxVzfKL9NT7k4zDH
cdeust added a commit that referenced this pull request Sep 16, 2026
@cdeust @claude
ci(test-sqlite): set the job budget from its measured p90 (#594)
6522f45 
The 10-minute budget left 1.7x headroom over this job's p90, and two runs
of 2026-09-16 exceeded it: main at a981f93, cancelled at 616s having
reached 74% of the suite, which made CI Green fail on
test-sqlite='cancelled'; and PR #584's first attempt at 610s. Both passed
on a rerun, in 416s and 276s, with no code change. The main run's own
tree had taken 266s as the pull request of the commit it squashed.

Measured over that day's 21 job executions, every attempt counted: 18
successes from 234s to 416s, median 328s, p90 348s, and three
non-successes: the two timeouts above and a 47s cancellation from a
superseded push. The budget is now 20 minutes, 3.4x that p90, about twice
the point where both timeouts fired, and one eighteenth of GitHub's own
ceiling, with the measurement recorded beside the number so suite growth
into the new headroom stays visible (ADR-1074). The Windows and matrix
budgets are untouched: nothing measured here applies to them.

An earlier draft of this commit, its ADR and its CHANGELOG entry cited 29
runs, 27 successes and a second cancellation from a superseded push. That
sample crossed several days and counted only each run's latest attempt,
so it hid the two reruns and invented the 150s cancellation. The review
of PR #595 caught it; the numbers above are the reproducible ones.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01Cz7A6goxVzfKL9NT7k4zDH
cdeust added a commit that referenced this pull request Sep 16, 2026
@cdeust @claude
ci(test-sqlite): set the job budget from its measured p90 (#594) (#595)
10b98d0 
The 10-minute budget left 1.7x headroom over this job's p90, and two runs
of 2026-09-16 exceeded it: main at a981f93, cancelled at 616s having
reached 74% of the suite, which made CI Green fail on
test-sqlite='cancelled'; and PR #584's first attempt at 610s. Both passed
on a rerun, in 416s and 276s, with no code change. The main run's own
tree had taken 266s as the pull request of the commit it squashed.

Measured over that day's 21 job executions, every attempt counted: 18
successes from 234s to 416s, median 328s, p90 348s, and three
non-successes: the two timeouts above and a 47s cancellation from a
superseded push. The budget is now 20 minutes, 3.4x that p90, about twice
the point where both timeouts fired, and one eighteenth of GitHub's own
ceiling, with the measurement recorded beside the number so suite growth
into the new headroom stays visible (ADR-1074). The Windows and matrix
budgets are untouched: nothing measured here applies to them.

An earlier draft of this commit, its ADR and its CHANGELOG entry cited 29
runs, 27 successes and a second cancellation from a superseded push. That
sample crossed several days and counted only each run's latest attempt,
so it hid the two reruns and invented the 150s cancellation. The review
of PR #595 caught it; the numbers above are the reproducible ones.


Claude-Session: https://claude.ai/code/session_01Cz7A6goxVzfKL9NT7k4zDH

Co-authored-by: Claude Opus 5 (1M context) <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

1 participant

@cdeust