Skip to content

refactor: carve distill.py's DSL-escaping helpers into escaping.py (zero-behavior slice of #59) #195

Description

@alphacrack

User story

As a new contributor, I want the first slice of the distill.py split to be a small, obviously-safe move that touches no grounding code, so that I can land the module-plus-re-export pattern before anyone attempts the risky grounding and guide splits in #59.

What

src/readme2demo/distill.py is 798 lines. This issue moves exactly five things — the helpers that escape/quote text for the two DSLs distill emits (VHS tape syntax, grep -E in bash) — into a new src/readme2demo/escaping.py, and re-exports them from distill so every existing import keeps working.

In scope — move these, byte-for-byte, no edits to the bodies:

Symbol Current location What it is
DistillError distill.py:68-69 class DistillError(RuntimeError)
_grep_flags_and_pattern distill.py:343-352 (?i)-prefix → grep -qiE translation
_VHS_REGEX_METAS distill.py:450 `set("\.+*?()
vhs_wait_pattern distill.py:452-462 escapes Go-regexp metacharacters, truncates to 40
vhs_quote distill.py:465-480 picks a VHS string delimiter ("`')

escaping.py must not import distill — that is the whole reason DistillError moves with them (vhs_quote raises it, and tape_from_guide catches it). Importing it back into distill preserves class identity, so except DistillError, pytest.raises(DistillError), and isinstance all behave identically. Only __module__ changes.

distill.py then gains one import line near the top, using the re-export convention already established at src/readme2demo/engines/__init__.py:1:

from readme2demo.escaping import (  # noqa: F401 — re-export, callers import from distill
    DistillError,
    _VHS_REGEX_METAS,
    _grep_flags_and_pattern,
    vhs_quote,
    vhs_wait_pattern,
)

The # noqa: F401 is load-bearing: pyproject.toml:96 sets select = ["E9", "F"], so pyflakes will flag the unused re-exports and fail CI without it.

Explicitly out of scope — do NOT move (grounding)

CLAUDE.md's one non-negotiable invariant is "The LLM never publishes anything a fresh container didn't independently execute… Grounding is enforced in code, not prompts." Everything that participates in that decision stays in distill.py and is off-limits in this PR:

  • heredoc_prefix (:53), _canonicalize_segment (:85), normalize_cmd (:109), _strip_env_prefix (:134), _grounded_candidates (:143), _segment_grounded (:171), is_grounded (:184), validate_grounding (:225), _collect_violations (:230)
  • _tolerate_findings_steps (:355) — failure classes 4 and 13; it calls normalize_cmd and reads entry.findings_success
  • parse_guide_steps (:544) and tape_from_guide (:594) — these belong to the guide.py slice of refactor: split distill.py (~800 lines) into grounding / artifacts / guide modules #59; parse_guide_steps also has external importers (orchestrator.py:171, tests/test_tutorial.py:460)
  • _render_commands_sh (:384), write_tape (:492), _tape_fetches_code (:740), build_tape_from_step_by_step (:757) — the artifacts.py slice

Also out of scope: fixing anything you move. _grep_flags_and_pattern only translates (?i) and mishandles other inline flags — that is a real bug, and it is #108's job, not this PR's. Move it broken.

Why

Slice of #59 ("refactor: split distill.py (~800 lines) into grounding / artifacts / guide modules"), which is currently a single large, unassigned refactor that a newcomer cannot safely pick up: three of its four target modules carry the grounding invariant, and a subtle mistake there is exactly the failure this project exists to prevent.

These five symbols are the only part of distill.py with zero grounding surface — none of them takes a CommandLog, none of them decides whether a command is proven. Landing them first proves out the new-module-plus-re-export mechanics against a green suite, so the reviewer of the scarier slices is reviewing only grounding logic, not import plumbing.

Grounding implication (flagged per repo policy)

Net effect on grounding: none, but two of the moved functions sit downstream of grounding decisions and must therefore be moved without any edit:

  1. vhs_quote raising is a drop decision. In tape_from_guide (distill.py:668-672 and 677-680), a step whose text can't be quoted is silently skipped and never reaches demo.tape. If the exception type changed, the except DistillError would stop catching, and an unquotable step would crash the render instead of being dropped. Preserving class identity via the re-export is what keeps this exact.
  2. _grep_flags_and_pattern feeds the verify gate. Its output lands in the commands.sh success-criteria assertion (distill.py:426). A behavior change here weakens the fresh-container check — the one source of "verified".

Because both are pure str → str with no log access, a literal move cannot change either. That is the property the acceptance criteria below are designed to make reviewer-checkable.

Pointers

Verified by reading the files at 64a3cbd:

  • src/readme2demo/distill.py:68class DistillError(RuntimeError), docstring "Raised when the distiller cannot produce a fully grounded output." Raised at :333 (run_distiller) and :478 (vhs_quote); caught at :671 and :679.
  • src/readme2demo/distill.py:343-352_grep_flags_and_pattern; sole caller is _render_commands_sh at :426. No test references it by name.
  • src/readme2demo/distill.py:448-462_VHS_REGEX_METAS and vhs_wait_pattern.
  • src/readme2demo/distill.py:465-480vhs_quote.
  • src/readme2demo/distill.py:514-515 — both are registered as Jinja filters inside write_tape: env.filters["vhs_quote"] = vhs_quote / env.filters["vhs_wait"] = vhs_wait_pattern. These lines stay put and keep referring to the re-exported names.
  • src/readme2demo/templates/demo.tape.j2:43,53,59 — the template applies | vhs_quote. Worth knowing: it never applies vhs_wait — the tape uses bare Wait (lines 38, 56, 61), consistent with failure class 8 ("Wait+Screen … never use"). So vhs_wait_pattern has no production caller today; only the filter registration and tests/test_distill.py:475-484 exercise it. Moving it is risk-free; deleting it is a separate judgement call and is not part of this issue.
  • tests/test_distill.py:18-24 imports DistillError from readme2demo.distill; :466 and :476 do function-local imports of vhs_quote / vhs_wait_pattern from readme2demo.distill. All three keep working through the re-export with no test edits at all.
  • src/readme2demo/engines/__init__.py:1 — existing # noqa: F401 re-export precedent to copy.
  • pyproject.toml:88-96line-length = 100, select = ["E9", "F"].

Acceptance criteria

  • New file src/readme2demo/escaping.py contains exactly the five symbols listed above, with their docstrings and comments carried over verbatim.

  • escaping.py imports nothing from readme2demo.distill (no cycle) — its only imports are re and from __future__ import annotations.

  • distill.py re-exports all five with # noqa: F401; distill.DistillError, distill.vhs_quote, distill.vhs_wait_pattern, distill._grep_flags_and_pattern, and distill._VHS_REGEX_METAS all still resolve.

  • Zero test changes. tests/test_distill.py is not modified — not even its imports. (Optionally you may add a single new assertion that distill.vhs_quote is escaping.vhs_quote; nothing more. No Regression: test is required — this is a refactor, not a bug fix.)

  • Full suite green: python -m pytest tests/ -q, all 47 distill tests included.

  • ruff check . clean.

  • No symbol from the out-of-scope list above appears in escaping.py.

  • Reviewer-checkable "only moves" property — the diff of the removed lines equals the diff of the added lines, modulo the new module's boilerplate:

    git diff main -- src/readme2demo/distill.py  | grep '^-' | grep -v '^---' | sed 's/^-//' | sort > /tmp/removed
    git diff main -- src/readme2demo/escaping.py | grep '^+' | grep -v '^+++' | sed 's/^+//' | sort > /tmp/added
    diff /tmp/removed /tmp/added

    The only differences should be the new module docstring, from __future__ import annotations, import re, and the re-export block added to distill.py. Paste that diff output in the PR description. If it shows a changed function body, the PR is doing more than it claims.

Notes for contributors

Prerequisites: none beyond Python. This is pure Python — no Docker, no API key, no network, no LLM call. Nothing you touch runs a container or talks to a model.

pip install -e ".[dev]"
python -m pytest tests/ -q     # ~1-2s, fully offline
ruff check .

Python ≥3.10; repo conventions are type hints + docstrings on public functions (both already present on everything you're moving — just carry them across).

Important

If in doubt, ask before moving a function. The five symbols above are the complete list, chosen because they are pure string manipulation with no grounding surface. If while working you think "this other helper looks pure too, I'll take it along" — stop and comment on this issue instead. normalize_cmd, heredoc_prefix, and friends look like innocent string helpers and are in fact the load-bearing grounding code (failure class 5 in CLAUDE.md: equivalences must be added symmetrically and tested in both directions). A PR that grows past this list will be asked to shrink back, so it costs you nothing to ask first.

Coordination: #47 proposes a textnorm.py for the shared command-normalization helpers. That is a different module with a different job (normalization semantics, grounding-adjacent) and deliberately does not overlap with escaping.py (quoting/escaping, no grounding). Naming was picked to keep them distinct — please don't merge the two ideas. #48 also edits distill.py signatures, so expect a trivial rebase if it lands first.

Metadata

Metadata

Assignees

No one assigned

    Labels

    area:pipelineingest/normalize/distill/verify/orchestratorgood first issueSmall, self-contained, newcomer-friendlytech-debtRefactor / cleanup, no behavior change intended

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions