Orchestrata Ultimate › Plans › ctok-p3-migration-map

P3 Migration Map — Replacing LibCST in the Codemod Engine

Source: plans/ctok-p3-migration-map.md at 8a2cb42ad · last changed 2026-08-04 · first committed 2026-08-04 · 252 lines.

Prose pending.

Plan text

P3 Migration Map — Replacing LibCST in the Codemod Engine

Scope: every libcst/cst.* touch-point under src/orchestrata_ultimate/refactor/codemod/ (confirmed via grep -rl "^import libcst\|^from libcst\|import libcst as cst" src/ — the LibCST footprint in src/ is exactly these 7 files, nothing outside refactor/codemod/, and refactor/rename/ — including EditApplier — is already 100% LibCST-free, confirming the contract's precedent claim):

• passes.py (827 lines)
• review.py (534 lines)
• _cig_builder.py (558 lines) vs _cig_builder_ast.py (409 lines, already LibCST-free)
• workspace.py (428 lines)
• txn.py (349 lines)
• report.py (125 lines)
• _parse_cache.py (97 lines)

Read-only audit. Ratified design: plans/contracts/ctok-parser.contract.json + plans/contracts/ctok-parser.mmd. No files under scripts/_ctok/, src/orchestrata_ultimate/refactor/ctok/, or scripts/ctok_roundtrip_gate.py were touched or read (out of scope, owned by the parallel C-tokenizer builder track).

0. Three findings that change the shape of P3 — read before the table

Finding 1 — "40 review checks" is spec drift, not implemented reality (HIGH)

specs/110-codemod-engine/spec.md:135 titles US-110-004 "Run 40-check in-memory code review without subprocess calls", and spec.md:1462-1471 describes a 3-phase pipeline (Phase 1 structural, Phase 2 graph-dependent, Phase 3 Prolog) implying dozens of discrete checks. The actual code ships exactly 4 ReviewPass classes — ParseIntegrity, BannedConstructs, DiffBudget, PrologSemanticReview (review.py:148,201,298,361) — matching spec.md:774 ("The engine SHALL ship 4 built-in review passes"), which directly contradicts the US-110-004 title two pages earlier in the same spec file. BannedConstructs bundles 3 sub-checks (_check_os_environ, _check_mocks, _check_banned_imports, review.py:225,250,272) with a docstring admission it "re-hosts checks from verification/review/checks_safety.py" — but only that one file's checks, not the 40-ish checks spread across verification/review/checks_{correctness,quality,safety,structure}.py. P3 has 4 review-pass classes + ~6 sub-checks to port, not 40. The migration table below is scoped to what exists, not the spec's aspirational title. Suggested owner: whoever owns spec-doc hygiene should also resolve the US-110-004 title/REQ-110-012 contradiction — separate from this port.

Finding 2 — cig_backend default is silently still "cst" in production paths (HIGH)

The ctok-parser.contract.json "reuse" section asserts _cig_builder_ast.py is "already prod default cig_backend='ast', no libcst." This is false as of the code read for this audit:

• CodemodWorkspace.load(..., cig_backend: str = "cst") — workspace.py:113 — the parameter defaults to "cst" (the LibCST-walking _cig_builder.py path), not "ast".
• scripts/codemod.py calls .load() at 4 call sites (lines 226, 230, 249, 359), none pass cig_backend → all 4 silently use the "cst" (LibCST) backend today.
• server.py:394-397's "reload" JSON-RPC method also calls .load() with no cig_backend → the live daemon's runtime reload path also uses the LibCST backend.
• Only _daemon_entry.py (2 call sites, initial boot + inotify-triggered incremental reload) explicitly passes cig_backend="ast".

Net effect: the same running daemon process uses the AST backend at boot but silently regresses to the LibCST backend on every manual "reload" RPC — an existing internal inconsistency, independent of P3. And the CLI (scripts/codemod.py, the primary tool entry point per docs/CODEBASE_MAP.md) is on the LibCST backend by default right now. P3 must not just delete _cig_builder.py — it must also delete the cig_backend parameter entirely (once there is only one backend, a selector parameter is dead weight) and verify no caller anywhere still assumes cig_backend="cst" is available. This closes both the contract's false claim and the boot/reload inconsistency in one motion.

Finding 3 — RemoveUnusedImportPass silently drops used names in multi-name imports, and the test suite has a coverage gap that hides it (HIGH — real bug, not a migration risk)

_ImportExtractor.visit_Import (passes.py:503-509) appends one (module_name, None, node) tuple per alias, but node is always the same statement-level cst.Import/cst.ImportFrom object — never the individual alias. RemoveUnusedImportPass.apply_file then collects any statement into unused if any one of its aliases is unused (passes.py:557-565), and _remove_imports deletes the whole statement by node identity (passes.py:588-614). Concretely: import os, sys where only os is unused deletes both os and sys, silently breaking any code that used sys. Same defect applies to from x import a, b.

The existing test suite (tests/refactor/codemod/test_transform_passes.py:240-300, TestRemoveUnusedImportPass, 6 tests) never exercises this shape — test_multiple_unused (line 273) uses three separate single-name statements (import os\nimport json\nimport sys\n), not one combined import os, sys. This is a textbook false-confidence gap: the pass "looks" well-tested (6 green tests) but the one input shape that triggers the real defect is never constructed. P3 must touch this exact logic to port it to ast/token walks — fix the defect in the same motion (per this project's "fix on sight" standard) and add a regression test for import os, sys / from x import a, b with mixed usage. Suggested owner: builder, inside the passes.py work-unit (Unit 2 below) — do not defer to a separate ticket, the porting work already requires rewriting _ImportExtractor/_remove_imports.

1. Migration table

2. _cig_builder_ast.py coverage of _cig_builder.py — verified, not assumed

tests/refactor/codemod/test_cig_builder_ast.py (292 lines) already contains a real parity suite: test_rich_module_structure_identical, test_test_file_detection_identical, test_multi_module_edges_identical, and — most directly — test_properties_identical_except_complex_annotations (line 125) and test_ast_and_cst_backends_produce_same_structure (line 247), which construct both a build_cig(forest) and build_cig_ast(sources) from the same source and assert set(cig_cst.nodes) == set(cig_ast.nodes) and cig_cst.key_to_id == cig_ast.key_to_id (lines 100, 110). The one documented, tested, accepted divergence is complex type-annotation strings (ast.unparse vs LibCST's own repr) — test_complex_annotation_uses_unparse (line 177).

Verdict: yes, _cig_builder_ast.py fully covers _cig_builder.py's output, with that one named and already-tested exception. _cig_builder.py can be deleted outright once Finding 2's cig_backend parameter is removed (§1 row 24) — there is nothing left in it worth keeping. Note build_cig_ast(sources: dict[str, str], ...) takes source text, not a parsed tree, unlike build_cig(forest: dict[str, cst.Module], ...) — the workspace-load wiring in row 22-24 already has to produce/keep raw source text alongside the LosslessModule for this call, which it does naturally (LosslessModule.stream.source per the contract's dataclass).

3. Fan-out plan — P3 swarm work-units

File-disjoint by construction (no two units write the same file). One serial gate, then four parallel units, then a non-editing integration/verification pass.

Unit 0 — report.py (serial gate, land first, blocks everything else) Swap ChangeEntry.original_tree/mutated_tree from libcst.Module to LosslessModule (row 27). Every other unit's pass/check/txn code references these two fields — this must merge before Units 1-4 start, or all four get immediate type conflicts. ~30 min, effort S, but it is the highest-blast-radius single edit in the whole migration (§1 row 27's risk note).

Unit 1 — workspace.py + _parse_cache.py + _cig_builder.py (delete) + _daemon_entry.py (drop redundant kwarg) Rows 21-25, 28. Owns "how the forest + CIG get built and cached." Deletes _cig_builder.py outright (§2 verdict), deletes the cig_backend parameter from CodemodWorkspace.load (closing Finding 2), updates the 2 _daemon_entry.py call sites (the cig_backend="ast" kwarg becomes dead once there's only one backend). Tests: test_workspace_load.py, test_benchmark_load.py, test_parse_cache.py, test_cig_builder_ast.py (repurpose the cst-vs-ast parity tests into single-backend sanity tests once _cig_builder.py is gone). Effort L — biggest structural unit, and the one that must re-verify scripts/codemod.py's 4 call sites and server.py's "reload" RPC no longer silently diverge (Finding 2).

Unit 2 — passes.py Rows 1-14. All 7 transform-pass classes + private visitor/transformer helpers in one file. Deletes _LSPToCSTTransformer; reworks RenameSymbolPass to consume SPEC-109 TextEdit ranges directly. Fixes Finding 3 (_ImportExtractor/_remove_imports multi-alias bug) as part of the same rewrite, plus adds the missing regression test. Test file: test_transform_passes.py (538 lines) — full fixture rewrite from cst.parse_module(...) to LosslessModule.parse(...). Effort L — largest file, most passes, the one real behavior fix.

Unit 3 — review.py Rows 15-20. 4 review-pass classes + 2 visitors. Mostly straightforward ast-walk swaps; 3 of the 6 checks (_check_mocks, DiffBudget, PrologSemanticReview) need no logic change at all, only need ChangeEntry (Unit 0) to already be LosslessModule-typed. Fixes the _position line-number bug in _OsEnvironVisitor/_BannedImportVisitor as a free side effect of the ast walk (row 16-17). Test file: test_review_passes.py (847 lines, the largest test file in scope) — fixture rewrite. Effort M.

Unit 4 — txn.py Row 26. One touch-point, trivial cst.parse_module → ast.parse swap. Test file: test_transactional_writeback.py (484 lines) — fixture rewrite. Effort S, lowest risk unit; good candidate to run first among the parallel four if the swarm wants an early green signal.

Integration pass (not a file-editing unit — verification only, run after Units 1-4 land) Confirm scripts/codemod.py, server.py, client.py, persistence/repository.py need zero code changes (confirmed in this audit: none of them import libcst directly — they're already insulated by the report.py/workspace.py type boundary) and that the full tests/refactor/codemod/ + tests/refactor/rename/ suites pass green, per REQ-110-126's acceptance criterion. Also re-run grep -rl "^import libcst\|^from libcst" src/ and assert zero results, then libcst removed from pyproject.toml:48 and check-deps green (REQ-110-125's acceptance).

4. Port category counts

(Row counts: 28 catalogued symbols/units across 7 files; _cig_builder.py's 39 internal symbols are counted as one delete row since the whole file goes.)

5. Anything the token+ast model genuinely can't express?

No — none found. The two categories that looked hardest going in — AddImportPass (insert a whole new statement) and RemoveUnusedImportPass (remove whole statements) — are both fully expressible as EditBuffer byte-splices: an insertion is a zero-width splice at a computed byte offset (derived from the token span of the "last import in group" statement, same grouping logic _find_insert_position already computes, just resolved to a byte offset instead of a body-list index), and a removal is a splice replacing a statement's full token byte-span with empty bytes. Nothing in the catalogued 28 rows requires CST-specific concrete-syntax fidelity that the ast-overlay + token-array model doesn't already carry — the contract's own design (NodeIndex mapping every ast node to its exact token byte-span) was built precisely to cover this case. The one place that looked like it might need "real" CST metadata — _LSPToCSTTransformer's PositionProvider line/col-to-node matching — turns out to be strictly easier in the new model (LSP TextEdits are already line/col ranges; the token stream's byte-offset index resolves them directly, no tree-walk-and-match needed), which is why it's delete rather than ast-walk.

6. What this audit did not cover

• The C tokenizer itself, TokenStream, NodeIndex, EditBuffer, LosslessModule — out of scope per the task boundary (parallel builder track, files not touched or read).
• verification/review/checks_{safety,quality,correctness,structure}.py — read only far enough to confirm BannedConstructs re-hosts from checks_safety.py specifically (Finding 1); did not catalog those modules' own LibCST usage (if any) since they are outside refactor/codemod/ and not named in the brief's file list. A quick check found no libcst import in the whole src/ tree outside refactor/codemod/, so verification/review/checks_*.py are confirmed to already be LibCST-free — but I did not deep-read their bodies.
• scripts/rename_engine.py, refactor/rename/* internals beyond confirming zero libcst imports — the contract already treats these as reused/unchanged.
• Test files outside tests/refactor/codemod/ (5 files identified importing libcst directly: test_cig_builder_ast.py, test_parse_cache.py, test_review_passes.py, test_transactional_writeback.py, test_transform_passes.py — all named in §3's fan-out plan, but I did not read every test body, only representative fixtures to confirm the cst.parse_module(...) fixture pattern and the Finding 3 coverage gap).

Related

• Plans
• Orchestrata Ultimate — project index
• Workflow
• Tasks