Orchestrata Ultimate › Plans › briefs › spec-110-s3-fix3-builder

SPEC-110 packet S3 — fix round 3 builder brief (round-3 reviewer findings F1, F2, F3)

Source: plans/briefs/spec-110-s3-fix3-builder.md at 8a2cb42ad · last changed 2026-10-07 · first committed 2026-10-07 · 917 lines.

Prose pending.

Plan text

SPEC-110 packet S3 — fix round 3 builder brief (round-3 reviewer findings F1, F2, F3)

Authority: ADR-0068 (Accepted) for REQ-110-034; contract plans/contracts/spec-110-codemod-engine.contract.json (9f7ee385d) sections classes (FR-012), check_catalogue, reuse, design_decisions D23 and D27; plan plans/spec-110-completion-plan.md §1 D5/D6/D7, §2 row S3, §3 ship-log S3 row; spec specs/110-codemod-engine/spec.md REQ-110-033/034/035, ERR-110-002. The baseline default is ADR-0073 (Proposed, Mohamed's ruling pending): do not flip it, do not add a CLI --baseline flag. Base: branch feat/spec-110-completion in /home/mohamed/workspace/orchestrata-ultimate-worktrees/wt-SPEC-110 at 325a0404b (S3 build 24b856ccd/81c5ea708/32a56e4d9; fix round 1 ace6149cb/32b00b303/13f063277; fix round 2 1e8c05eb7/325a0404b; ADR-0073 commits 2b8e510c0/ec696caf1). No rebase or amend; commits on top. Previous briefs: plans/briefs/spec-110-s3-builder.md (build). Round-3 reviewer report: summarised in §4 below with every measured number.

DESIGN_WAIVER: ADR-0068 (Accepted) for REQ-110-034; RUL-022 contract plans/contracts/spec-110-codemod-engine.contract.json (9f7ee385d, D27) for the S3 classes and the commit() wiring; plan D5/D6/D7. Scope beyond them is a STOP, not a guess.

GOAL: Close round-3 findings F1 (include-root normalisation drops every CO_LOCAL_MODULE_EXISTS finding for an absolute, . or .. include), F2 (CO_THIRD_PARTY_DECLARED reports sibling modules directly under scripts/ as undeclared third-party packages at error severity) and F3 (one docs sentence the real tree contradicts), prove each with the reviewer's probes, re-measure the whole tree once, and record the numbers in the plan S3 row.

1. Exact source under change (serialize_codebase_ast.py source, byte-exact at 325a0404b)

1a. refactor.codemod.workspace._include_root — F1 site

# src/orchestrata_ultimate/refactor/codemod/workspace.py:218-220
def _include_root(subdir: str) -> str:
    """``subdir`` as a repo-relative POSIX root without a trailing slash."""
    return os.path.normpath(subdir).replace(os.sep, "/")

Caller (CodemodWorkspace.load, workspace.py:411): include_roots=tuple(_include_root(subdir) for subdir in include). How load walks the same include (lines 40-46 of the method body):

        files: list[Path] = []
        for subdir in include:
            subdir_path = root / subdir
            if subdir_path.exists():
                files.extend(sorted(subdir_path.rglob("*.py")))

root / subdir makes an absolute subdir win over root (pathlib semantics), so the files load correctly while the stored root stays absolute and never matches a repo-relative candidate path.

1b. refactor.codemod.review_checks._outside_include_roots — F1 consumer

# src/orchestrata_ultimate/refactor/codemod/review_checks.py:153-171
def _outside_include_roots(message: str, roots: tuple[str, ...]) -> bool:
    """Whether the import a completeness finding names lies outside ``roots``. ..."""
    found = _IMPORT_IN_MESSAGE.search(message)
    if not roots or found is None:
        return False
    full = found.group(1)
    candidates = _build_module_candidates(full.split(".")[0], full)
    return not any(
        candidate.replace(os.sep, "/").startswith(root + "/")
        for candidate in candidates
        for root in roots
    )

Used once, review_checks.py:358 inside ReusedCheck.run. Candidate paths are repo-relative (src/orchestrata_ultimate/...), so a root of /abs/..., . or .. matches nothing and EVERY finding is dropped (the reviewer measured 0 findings where 1 is expected).

1c. refactor.codemod.review_engine._top_level — F2 site

# src/orchestrata_ultimate/refactor/codemod/review_engine.py:335-340
def _top_level(path: str) -> str:
    """The importable top-level name a forest path contributes."""
    parts = path.removesuffix(".py").split("/")
    if parts[0] == "src" and len(parts) > 1:
        return parts[1]
    return parts[0]

Caller: InMemoryReviewEngine._shared (review_engine.py:259-270):

        paths = set(forest) | {path for path, _ in ws.unparsable}
        return {
            "project_modules": frozenset(_top_level(path) for path in paths),
            "workspace_files": frozenset(paths),
            "declared_deps": self._declared_dependencies(ws.root),
            "banned_libraries": _banned_libraries(),
        }

_top_level("scripts/generate_rtm_evidence.py") is "scripts", so import generate_rtm_evidence from a sibling script is neither project nor declared → error. check-deps in scripts/serialize_codebase_ast.py already solves this with _local_module_names (every module and package stem under src scripts tests claude is local); mirror that rule, do not import scripts/ from src/.

2. Symbol references (refs, live AST at 325a0404b)

Who imports the three modules: workspace has 36 import sites (production: __init__.py:55, _daemon_entry.py:95, review_engine.py:44, server.py:18; the rest tests); review_engine 8 (__init__.py:51, review_catalogue.py:26, review_checks.py:65, workspace.py:55, 4 tests); review_checks 3 (review_catalogue.py:405, 2 test imports). Production constructions of CodemodWorkspace all go through load(); _daemon_entry.py:102,121 and server.py:416 are full loads. Full pack: /tmp/claude-1001/-home-mohamed/42ea036c-e296-4b26-b447-2f612e9dd49a/scratchpad/s3_fix3_pack.md. Note: the Prolog code-intelligence KB (scripts/code_intel.py) predates the S3 symbols (it answers unknown_symbol for _top_level); use refs/who-imports from the live toolkit, not code_intel.

3. F1 input table — what load() stores today and what it must store

CLI passes --include through unchanged (scripts/codemod.py:228-231). root is the repo root.

Rule: (root / subdir).resolve().relative_to(root.resolve()), POSIX-joined; Path(".") relative → no restriction; ValueError from relative_to → raise. Keep _include_root a module function (the test at test_review_engine.py:722 style imports privates); give it the root argument.

4. F2 affected sites (reviewer's whole-tree identity review at 325a0404b)

CO_THIRD_PARTY_DECLARED errors that are sibling scripts/ modules (all exist as scripts/*.py):

Fix: a file directly under a non-src root (scripts/foo.py, tests/foo.py, claude/foo.py) contributes its basename stem foo; a package directory directly under such a root contributes its directory name; src/<pkg>/... keeps contributing <pkg>. Do NOT make every nested stem local (that would silence real third-party misses); mirror exactly what _local_module_names in scripts/serialize_codebase_ast.py does — read it with source first and copy the rule, not the function.

Not F2, leave alone and do not re-classify: src/orchestrata_ultimate/gateway/config.py:59 (true positive), scripts/attribute_traces_runtime.py:82 coverage (arguable, carried), the vendored scripts/_janus_patched/** hits, scripts/check_staged_secrets.py:297, scripts/serialize_codebase_ast.py:6957.

5. F3 docs sentence

docs/codemod-review-engine.md lines 162-178, paragraph "Narrow include.". The sentence "A full load stores src, scripts and tests, so a missing module under those is reported." is false on this tree: the reused _build_module_candidates includes src/orchestrata_ultimate/__init__.py, which any wide include loads, so CO_LOCAL_MODULE_EXISTS never fires for orchestrata_ultimate.* imports on a full load, on --include src, or on --include src/orchestrata_ultimate (reviewer: core.nope_r3 and analysis.nope_r3 appended under include=("src",) → 0 findings). Replace it with the measured truth: a full load stores the three roots and drops nothing, and the check reports a missing module only when the package's __init__.py is NOT in the forest (a reused-function property, carried). Keep the closing caveat; make the two agree. Rename or re-docstring tests/refactor/codemod/test_review_checks.py::test_local_module_check_is_unrestricted_on_a_full_load so its name says what it proves (nothing is dropped), not that a missing module is reported.

6. Probes — absolute paths, commands, measured baselines, expected after

All under S=/tmp/claude-1001/-home-mohamed/42ea036c-e296-4b26-b447-2f612e9dd49a/scratchpad. Run each from the worktree with .venv/bin/python. Check scripts/observe_pytest.py list first; the whole-tree probe needs an idle box (~110 s, ~3.4 GB).

7. Tests to add (one function per row, real parsed modules, no doubles)

tests/refactor/codemod/test_workspace_load.py:

• test_include_root_is_stored_repo_relative_for_an_absolute_include (absolute path → src/.../analysis, 11 files loaded on the real tree or a temp repo)
• test_include_root_dot_stores_no_restriction (include=(".",) → include_roots == ())
• test_include_root_parent_traversal_inside_root_is_normalised (../<repo-name>/src → src)
• test_include_outside_the_repo_root_is_refused (ValueError names the include and the root)
• test_include_root_forms_src_slash_dot_slash_and_double_slash_still_normalise (parametrised over src/, ./src, src//x/)

tests/refactor/codemod/test_review_checks.py:

• test_absolute_include_root_still_reports_a_missing_in_scope_module (engine-level, mirrors r3_probe.py)
• rename/re-docstring test_local_module_check_is_unrestricted_on_a_full_load per §5

tests/refactor/codemod/test_review_engine.py:

• test_top_level_of_a_script_directly_under_scripts_is_its_stem (scripts/foo.py → foo; scripts/pkg/__init__.py → pkg; src/orchestrata_ultimate/x/y.py → orchestrata_ultimate; tests/conftest.py → conftest)
• test_sibling_script_import_is_not_an_undeclared_third_party (temp repo: scripts/a.py imports b, scripts/b.py exists → 0 CO_THIRD_PARTY_DECLARED; import numpy undeclared still flags)

@traces on every new test: REQ-110-034 for F1/F3 (substrate/scope), REQ-110-033 for F2 (the check's behaviour); nothing else (RUL-013).

8. Gates, commands, expected results

Run pytest in the background with a log and echo "rc=$?" >>; wait with timeout 290 bash -c 'until grep -q "^rc=" <log>; do sleep 10; done' (the wait command must not contain the tokens run_tests, passed, FAIL or a check-mark emoji). Never commit while a pytest run is in flight in this worktree (git-sentinel).

FILES: edit src/orchestrata_ultimate/refactor/codemod/workspace.py (_include_root + its call in load, nothing else), review_engine.py (_top_level), review_checks.py (only if _outside_include_roots needs the "no roots" convention adjusted; the drop rule itself is right), tests/refactor/codemod/test_workspace_load.py, test_review_checks.py, test_review_engine.py, docs/codemod-review-engine.md (§5), plans/spec-110-completion-plan.md (S3 ledger cell: "FIX ROUND 3" with the §6 numbers), reports/rtm-evidence.{md,json} (regenerate, unverified). Do not touch analysis/, review.py, verification/review/*, scripts/codemod.py, review_catalogue.py rows, the contract JSON, or core/patterns/observer.py.

CONSTRAINTS: worktree /home/mohamed/workspace/orchestrata-ultimate-worktrees/wt-SPEC-110, branch feat/spec-110-completion; never cd into the main checkout. Read Python only via .venv/bin/python scripts/serialize_codebase_ast.py source|symbols|refs|who-imports <qname> --no-daemon (qualified names omit orchestrata_ultimate.); Edit/Write tools for edits (hooks block Bash writes and cat/grep/sed on .py). No mocking/fakes/monkeypatch, no skip/xfail, no pytest-xdist, function-scope fixtures, absolute imports, os.environ.get, uv only. Commit as git -c user.name=Mohamed -c user.email=msamy@sufigeeks.org commit, no AI attribution, no "should" anywhere. Restore hook litter by name before each commit (docs/CODEBASE_MAP.md docs/CODEBASE_MAP_FULL.md plans/ast-ultimate.json plans/ast-deep-comparison.json plans/ast-tests.json reports/.ship-verified reports/run-tests-latest.json, only those present and modified). A fork the contract and D5/D6/D7 do not settle → STOP and report; do not guess.

DONE WHEN: (1) every §3 row behaves as its last column says and the §7 test_workspace_load.py tests pass; (2) the §4 files produce no CO_THIRD_PARTY_DECLARED error in the whole-tree probe and the two §7 _top_level tests pass; (3) docs/codemod-review-engine.md §5 sentence replaced and the test renamed; (4) all §6 probes give their "expected after" column, with the dry-run and whole-tree numbers written into the plan S3 ledger cell as "FIX ROUND 3"; (5) all §8 gates at their expected results, pytest groups green with zero skips at or above the baselines; (6) commits made, tree clean except hook litter; report with shas, the §6 table filled in, the pytest counts, the coverage table, and deviations with reasons.

VERIFY: the §6 table with measured values; pytest counts per group; git log --oneline 325a0404b..HEAD; git status --short.

Round 3b — the fork the round-3 builder stopped on (orchestrator's pick, 2026-10-07)

Round 3 (86859b5ce, 7b00d9d4b, a80a2c543) closed F1 and F3. F2 moved the seven sibling-script imports from CO_THIRD_PARTY_DECLARED to CO_LOCAL_MODULE_EXISTS: once generate_rtm_evidence is in project_modules, the reused check_file_completeness runs _build_module_candidates(top, full), which proposes src/<top>/... shapes and never scripts/<top>.py, so it reports "module file not found in workspace". Whole-tree errors stayed at 242.

Pick: cure (a). Drop such findings in ReusedCheck.run next to the include-root drop; do not widen the reused candidates (every other consumer of verification/review would change). The brief's F2 prose in §4 contradicted itself ("mirror _local_module_names" vs "do NOT make every nested stem local"); the round-3 builder followed the explicit prose, which is the right reading. Nested stems stay un-local; scripts/_janus_patched/setup.py:13 (_find_swipl, which lives at scripts/_janus_patched/janus/_find_swipl.py) is vendored and stays carried.

3b.1 Exact source at a80a2c543

# src/orchestrata_ultimate/refactor/codemod/review_checks.py:341-362
    def run(self, module: LosslessModule, ctx: ReviewContext) -> list[Finding]:
        ...
        findings = IssueAdapter.to_findings(
            self._fn(*self._args(module, ctx)), ctx.path, self.descriptor.severity, self.descriptor.id,
        )
        if self._arg_shape == "path_tree_files_modules":
            findings = [
                finding
                for finding in findings
                if not _outside_include_roots(finding.message, ctx.include_roots)
            ]
        if self._extension is not None:
            findings.extend(self._extension.run(module, ctx))
        return findings

_IMPORT_IN_MESSAGE = re.compile(r"Import '([^']+)'") (review_checks.py:120). ReviewContext fields available: workspace_files: frozenset[str] (forest-relative paths, parsed and unparsable), project_modules, include_roots, path. _SIBLING_ROOTS is defined in review_engine.py next to _top_level (review_engine.py:338-353); import it, do not duplicate the tuple.

3b.2 The rule

A finding of the path_tree_files_modules check is dropped when the import it names has a top-level name top such that, for some root in _SIBLING_ROOTS, f"{root}/{top}.py" or f"{root}/{top}/__init__.py" is in ctx.workspace_files. Implement as _resolves_to_sibling_module( message, workspace_files) -> bool beside _outside_include_roots, applied in the same comprehension. Nothing else: a top that is only a nested stem (scripts/_janus_patched/janus/_find_swipl.py) does not match, by design.

3b.3 Sites that must change, and the ones that must not

Also carried, record in the plan ledger cell, do not fix here: the DocstringSweepPass SystemError the round-3 builder hit with old=X new=Y on --include scripts (a pass defect: the edit corrupts a string and the native tokenizer raises) — "docstring-sweep with a substring that lands inside a string literal raises SystemError instead of a finding or a refusal; owner: S4/S5 hygiene".

3b.4 Tests (names; real parsed modules in a temp repo loaded through CodemodWorkspace.load)

tests/refactor/codemod/test_review_checks.py:

• test_sibling_script_import_present_in_the_forest_is_not_a_missing_local_module (scripts/a.py imports b; scripts/b.py exists → 0 CO_LOCAL_MODULE_EXISTS)
• test_sibling_package_import_present_in_the_forest_is_not_a_missing_local_module (scripts/a.py imports pkg; scripts/pkg/__init__.py exists → 0)
• test_import_with_no_sibling_file_is_untouched_by_the_sibling_rule (scripts/a.py imports gone; no scripts/gone.py or scripts/gone/__init__.py exists, so gone is not in project_modules; with requests declared so the dependency check runs, CO_THIRD_PARTY_DECLARED still fires for gone and the new rule drops nothing)
• test_nested_stem_under_a_sibling_root_is_not_treated_as_local (scripts/v/w/x.py; scripts/a.py imports x → the finding is kept)

@traces REQ-110-033 (the check's behaviour). Keep the round-3 _top_level tests as they are.

3b.5 Done when (3b)

(1) the four 3b.4 tests pass; (2) probe_engine_fix1.py gives 235 errors / 92 files with CO_LOCAL_MODULE_EXISTS = 1 (setup.py:13) and the by-rule table is written into the plan S3 ledger cell as "FIX ROUND 3b" together with the two carried items in 3b.3; (3) the §8 gates at their expected results, pytest groups at or above the round-3 counts (85 / 66 / 53 / 161), zero skips; coverage review_checks.py ≥ 99%; (4) docs/codemod-review-engine.md gains one sentence on the sibling rule in the "Narrow include" paragraph; (5) commits on top of a80a2c543 as Mohamed, tree clean except hook litter; report with shas, the 3b.3 table filled, pytest counts, deviations.

Round 4 — round-4 reviewer findings (orchestrator's picks, 2026-10-07)

Round 3b (ac003e9eb, 7eeb8bbcd, f25c0577b) closed the seven sibling-script false positives; the round-4 reviewer confirmed every number (235 errors / 93 files, five files cleared, none added) and returned NO-GO narrow on tests plus one design question. Picks:

• R4-1 (must-fix, tests): no test fails if the 3b drop is removed from ReusedCheck.run. The narrow-load tests (_review_after_append loads include=("scripts",)) never reach _resolves_to_sibling_module, because _outside_include_roots already drops the finding (the reused candidates for a bare import are b.py, b/__init__.py, src/b/__init__.py, none under scripts/), and the wide-load engine test asserts only on CO_THIRD_PARTY_DECLARED.
• R4-2 (design question → pick): sibling locality is importer-scoped. A module under src/ that imports b where only scripts/b.py exists is an ImportError in production (scripts/ is not on sys.path for src), so it stays a CO_THIRD_PARTY_DECLARED error; a module under a sibling root importing b is local. Also, import b.nope where scripts/b.py is a plain module is a genuinely missing submodule and is NOT dropped (the drop keys on the top name only today). This is an implementation refinement inside contract D27 (record it as a D27 note in the plan ledger cell), not an ADR.
• R4-3 (low → fix, one line): _include_root containment is lexical (normpath); a symlink inside the repo pointing outside is accepted and loads foreign files, while the docs say an include that "resolves outside the repo raises". Make the containment check use Path.resolve(); keep the lexical relative form as the stored root for the in-repo case.
• R4-4 (low → carried): _top_level("claude/tools/x.py") gives tools though claude/tools is the sys.path entry; the default include (src,scripts,tests) never loads claude. Record as a carried item; do not fix here.
• C5 priority raised (carried, record): the DocstringSweepPass defect is at passes.py:779 (wrong span on an implicitly concatenated f-string plus plain string, e.g. scripts/check_generated_artifacts.py:1274-1276, overwriting the closing quote); the native tokenizer at refactor/ctok/tokenizer.py:117 then raises SystemError instead of a SyntaxError. The same misplaced edit can silently corrupt a file whenever the result still parses. Owner: S4/S5 hygiene, priority high. Update the ledger wording.

4.1 Exact source at f25c0577b

# src/orchestrata_ultimate/refactor/codemod/review_checks.py:175-194
def _resolves_to_sibling_module(message: str, workspace_files: frozenset[str]) -> bool:
    found = _IMPORT_IN_MESSAGE.search(message)
    if found is None:
        return False
    top = found.group(1).split(".")[0]
    return any(
        f"{root}/{top}.py" in workspace_files
        or f"{root}/{top}/__init__.py" in workspace_files
        for root in _SIBLING_ROOTS
    )

# review_checks.py, inside ReusedCheck.run (the comprehension)
        if self._arg_shape == "path_tree_files_modules":
            findings = [
                finding
                for finding in findings
                if not _outside_include_roots(finding.message, ctx.include_roots)
                and not _resolves_to_sibling_module(finding.message, ctx.workspace_files)
            ]

# src/orchestrata_ultimate/refactor/codemod/review_engine.py:258-269  (_shared)
        paths = set(forest) | {path for path, _ in ws.unparsable}
        return {
            "project_modules": frozenset(_top_level(path) for path in paths),
            "workspace_files": frozenset(paths),
            "declared_deps": self._declared_dependencies(ws.root),
            "banned_libraries": _banned_libraries(),
        }

# review_engine.py:271-293  (_context) — builds ONE ReviewContext per reviewed path
        return ReviewContext(
            path=path, forest=forest, cig=ws.cig,
            project_modules=shared["project_modules"],
            workspace_files=shared["workspace_files"],
            declared_deps=shared["declared_deps"], root=ws.root,
            is_test=_is_test_path(path), is_main_file=name in _MAIN_FILES,
            banned_libraries=shared["banned_libraries"], include_roots=ws.include_roots,
        )

# src/orchestrata_ultimate/refactor/codemod/workspace.py:218-236
def _include_root(root: Path, subdir: str) -> str | None:
    rel = os.path.relpath(
        os.path.normpath(os.path.join(os.path.abspath(root), subdir)),
        os.path.abspath(root),
    ).replace(os.sep, "/")
    if rel == ".":
        return None
    if rel == ".." or rel.startswith("../"):
        raise ValueError(f"include {subdir!r} resolves outside the repo root {str(root)!r}")
    return rel

4.2 The changes

R4-2 implementation. In _shared, split the module names: "project_modules" keeps only the src/<pkg> names plus the sibling roots' own names (what _top_level returned before round 3 for src paths and for <root>/__init__.py); add "sibling_modules": frozenset of the stems and package names directly under each sibling root (what round 3 added). In _context, when path is under a sibling root (path.split("/")[0] in _SIBLING_ROOTS), pass project_modules = shared["project_modules"] | shared["sibling_modules"]; otherwise shared["project_modules"] alone. _top_level stays as it is (it is the per-path classifier); if it is cleaner, add _sibling_name(path) -> str | None beside it and keep _top_level returning the pre-round-3 answer — either way, test_top_level_of_a_script_directly_under_scripts_is_its_stem must be updated to assert the function it now tests, not a tautology. In _resolves_to_sibling_module(message, workspace_files, importer_path): return False unless importer_path.split("/")[0] in _SIBLING_ROOTS; and when the match is f"{root}/{top}.py" (a plain module) and the import has more than one component (found.group(1) contains .), return False (a submodule of a plain module cannot exist). Pass ctx.path from run.

R4-1 tests. In test_review_checks.py: _review_after_append loads include=("src", "scripts", "tests") (create empty src/ and tests/ dirs in the temp repo if needed) so the finding survives _outside_include_roots and only the sibling rule can drop it; every sibling test asserts [f for f in findings if f.rule == "CO_LOCAL_MODULE_EXISTS"] == [] (or != [] for the kept cases) explicitly. In test_review_engine.py test_sibling_script_import_is_not_an_undeclared_third_party asserts BOTH rules absent. Add:

• test_src_module_importing_a_scripts_sibling_is_still_an_undeclared_third_party (src/pkg/m.py imports b; only scripts/b.py exists; requests declared → one CO_THIRD_PARTY_DECLARED error for b, as at 325a0404b)
• test_submodule_of_a_plain_sibling_module_is_still_reported (scripts/a.py imports b.nope; scripts/b.py is a plain module → CO_LOCAL_MODULE_EXISTS finding kept)
• test_sibling_rule_is_the_only_thing_dropping_the_finding (wide load; the raw reused finding Import 'b' -- module file not found in workspace exists on the forest, the engine reports none; the reviewer's scratchpad/r4/sib_probe.py shows the shape)

R4-3. _include_root: compute resolved = (Path(root) / subdir).resolve() and refuse when resolved is not inside Path(root).resolve() (use relative_to in a try); keep returning the lexical rel for the accepted case so stored roots match forest paths. Test in test_workspace_load.py: a symlink ext -> <tmp outside the repo> as the include raises ValueError; a symlink alias -> src inside the repo is accepted and stores alias.

Records. Plan S3 ledger cell: "FIX ROUND 4" with the measurements below, the D27 note for R4-2, the R4-4 carried item, and the C5 wording update. docs/codemod-review-engine.md: one sentence that sibling locality applies to importers under a sibling root only.

4.3 Expected measurements after round 4

4.4 Done when (round 4)

(1) removing and not _resolves_to_sibling_module(...) from run makes at least two tests fail (do it locally, observe, restore — do not commit the removal); (2) the three new tests and the symlink tests pass; (3) probe_engine_fix1.py numbers reported, any new src/ true positive listed; (4) §8 gates at expected results; (5) ledger, D27 note, docs sentence written; (6) commits on top of f25c0577b as Mohamed, tree clean except hook litter; report with shas, the 4.3 table filled, pytest counts, deviations.

Round 5 — execute ADR-0073 (Accepted by Mohamed 2026-10-07, repo 570160a21, wiki bc14fbe)

Authority for this round is ADR-0073 itself (docs/adr/0073-err-110-002-amended-default-review-engine-aborts-on-introduced-findings-delta-baseline.md, Accepted); the DESIGN_WAIVER is ADR-0073 plus ADR-0068 and contract D23/D27. Plan D6 is superseded. Runs AFTER round 4 lands (same files); base is round 4's HEAD.

5.1 Exact source at the sites

# src/orchestrata_ultimate/refactor/codemod/review_engine.py:167-197  (InMemoryReviewEngine.__init__)
        baseline: _Baseline = "absolute",
        ...
            baseline: ``absolute`` reports every finding of the mutated
                modules (plan D6); ``delta`` only those the pass introduced.

# src/orchestrata_ultimate/refactor/codemod/workspace.py:773-797  (_resolve_reviews)
    engine = review_engine
    if engine is None and reviews is None:
        engine = InMemoryReviewEngine()

Spec row (specs/110-codemod-engine/spec.md:957): | ERR-110-002 | REQ-110-013 | Review pass produces error-severity finding | ERROR | Abort commit, return CommitReport with findings, write nothing to disk | and the commit() docstring in the spec at line 1098: ERR-110-002: If any review produces error-severity finding.

CLI run flags (scripts/codemod.py:71-116): --pass, --arg, --apply, --dry-run, --observe, --output; _cmd_run at line 334 builds the workspace and calls commit().

5.2 The changes

1. InMemoryReviewEngine.__init__: default baseline="delta"; docstring: delta (default, ADR-0073) reports only the findings the change introduced, matched on path, rule and line-free message; absolute reports every finding of the mutated modules (the clean-slate gate). Keep the ValueError on other values.
2. _resolve_reviews builds InMemoryReviewEngine() → now delta by default; no code change needed there, but the docstring of commit() (workspace.py) gains: "By default an error-severity finding aborts the commit only when the change introduced it (ADR-0073); pass review_engine=InMemoryReviewEngine(baseline='absolute') for the unconditional abort."
3. scripts/codemod.py run subcommand: --baseline {delta,absolute} (default delta), passed as review_engine=InMemoryReviewEngine(baseline=args.baseline) to commit(); help text names ADR-0073. The JSON report records baseline.
4. Spec specs/110-codemod-engine/spec.md:957 ERR-110-002 row: append to the Recovery cell: "For the default engine an error-severity finding means one the change INTRODUCED (delta baseline, ADR-0073); baseline='absolute' restores the unconditional abort." Line 1098 docstring: append "(introduced by the change, ADR-0073; absolute baseline aborts on any)". No other spec text.
5. Checklist specs/110-codemod-engine/checklists/requirements.md: REQ-110-033 and REQ-110-034 rows gain "(default baseline delta, ADR-0073)"; totals unchanged.
6. Plan plans/spec-110-completion-plan.md: D6 bullet (line 67) gains a trailing sentence "Superseded by ADR-0073 (Accepted 2026-10-07): the default is 'delta'; 'absolute' is the opt-in." — do not rewrite D6's history. S3 ledger cell: "ROUND 5 — ADR-0073 executed" with the measurements in 5.4.
7. Docs docs/codemod-review-engine.md: the baseline paragraph states the new default, the --baseline absolute escape, and the two documented delta limits (masking; measure-bearing warnings).
8. Tests:
   • tests/refactor/codemod/test_review_engine_commit_wiring.py: test_default_commit_writes_when_the_only_error_finding_is_legacy (temp repo with a file holding API_KEY = "sk-" + 24 alphanumerics on line 1; a pass appends an unrelated line → success is True, the file is written, the legacy finding is NOT in the report because delta subtracts it) and test_absolute_engine_still_aborts_on_a_legacy_error_finding (same repo, review_engine=InMemoryReviewEngine(baseline="absolute") → success is False, files_written == ()), and test_default_commit_aborts_when_the_change_introduces_an_error (pass adds the secret → success is False). @traces REQ-110-033, REQ-110-035.
   • tests/refactor/codemod/test_review_engine.py: the existing test_absolute_is_the_default_baseline_and_delta_reports_only_new_findings is renamed and re-asserted for the new default (InMemoryReviewEngine()._baseline == "delta"; the explicit baseline="absolute" path still works).
   • tests/scripts/test_codemod_cli.py: --baseline absolute on the existing review-abort fixture → rc 1; default on a repo whose only error is legacy and whose pass is unrelated → rc 0 and the file written with --apply; the report JSON carries "baseline".

5.3 Guard against the obvious regression

The round-2 reviewer's probe_delta.py (prepend import sys above a legacy os.environ["LEGACY"]) must give delta 0 / absolute 1; run it and report. The TST-110-025 mutated-forest proof and TST-110-026 event-order test must still pass unchanged (they assert findings and order, not the baseline).

5.4 Expected measurements

5.5 Done when (round 5)

(1) 5.2 items 1–8 done; (2) 5.3 probes as stated; (3) 5.4 table filled; (4) §8 gates at expected results, uv run python scripts/check_spec_authority.py rc 0 after the spec edit; (5) RTM regenerated (unverified); (6) commits as Mohamed on top of round 4's HEAD, tree clean except hook litter; report with shas, tables, counts, deviations.

Round 6 — round-6 reviewer blockers B1/B2 and must-fixes M1/M2 (orchestrator's picks, 2026-10-07)

Round 5 (7db0f25e1, 47e6b20d2, fc793fef2) executed ADR-0073. The round-6 reviewer confirmed the default, the wiring, 1,059 + 161 tests green, and returned NO-GO on:

• B1 (blocker, security regression). _delta keys on (path, rule, _line_free(message)). The secret check's message is content-free ("L<n>: Possible hardcoded secret."), so an IN-PLACE edit of a line that already carries that finding is invisible: replace-string old=PLACEHOLDER new=«redacted» on API_KEY = «redacted» writes under the default (rc 0, findings: []) where absoluterefuses (rc 1). Probe:/tmp/claude-1001/-home-mohamed/42ea036c-e296-4b26-b447-2f612e9dd49a/scratchpad/r6rev/inject_probe.py(case B-mask onscripts/check_staged_secrets.py: blank legacy line 297, append a new sk-secret → default 0 errors,absolute1 at line 384) andscratchpad/r6rev/clirepo/. **Pick: the reviewer's option 1.** The delta key gains the stripped source text of the finding's line in its own tree. An edited line's finding is, by the accepted decision's own words, a finding "the change INTRODUCED"; the refinement errs toward a false abort (a reformatted legacy line re-fires) rather than a silent write, and --baseline absoluteis unaffected. The ADR's Decision text names the key as(path, rule, message)` with line numbers stripped; the orchestrator records this refinement in the ADR's Provenance (lifecycle, not body) and in the docs.
• B2 (blocker, conformance to the ruling). The ADR Decision says "Pre-existing error findings are still reported (as findings on the CommitReport), they no longer veto the write." The code discards them (review() replaces findings with _delta(...)), and two tests assert the empty report. Pick: make the code match the accepted text. Legacy findings travel on the report in a separate field and never veto.
• M1. review_engine.py:12-14 module docstring and the checklist REQ-110-035 row (requirements.md:111) still say absolute is the default.
• M2. No CLI test covers the DEFAULT path aborting on an introduced error (the only CLI abort test now passes --baseline absolute).
• C1. The S3 ledger "ROUND 5" cell mixes a 236-error run's by-rule table and timings with the inferred 235 / 93. Measured by the reviewer at fc793fef2 with absolute: 235 errors / 93 files / 12,780 warnings; by rule SF_EVAL_EXEC 154, SF_FLUSHDB 29, TI_MODULE_FIXTURES 22, SF_ENVIRON_SUBSCRIPT 16, TI_GLOBAL_STATE 8, CO_THIRD_PARTY_DECLARED 6; load 35.6 s, review 82.4 s, 3,427 MB. Write these.
• C4 (carried, record): _resolves_to_sibling_module accepts any dotted name under a sibling PACKAGE (pkg.thing with only scripts/pkg/__init__.py), so a missing submodule of a sibling package is never reported. Side observation to record too: import os as x; x.environ["K"] is not flagged under either baseline (reused-check limitation).

6.1 Exact source at fc793fef2

# src/orchestrata_ultimate/refactor/codemod/review_engine.py:212-241  (review)
        mutated = {entry.path: entry.mutated_tree for entry in changes.entries}
        findings = self._run_checks(ws, mutated, {**ws.forest, **mutated})
        if self._baseline == "delta":
            original = {entry.path: entry.original_tree for entry in changes.entries}
            findings = self._delta(
                findings, self._run_checks(ws, original, {**ws.forest, **original})
            )
        if self._include_phase3:
            prolog = (self._prolog if self._prolog is not None else PrologSemanticReview())
            findings.extend(prolog.review(cast("Any", ws), changes))
        return findings

# review_engine.py:328-345  (_delta)
    def _delta(mutated: list[Finding], original: list[Finding]) -> list[Finding]:
        known = Counter((f.path, f.rule, _line_free(f.message)) for f in original)
        introduced: list[Finding] = []
        for finding in mutated:
            key = (finding.path, finding.rule, _line_free(finding.message))
            if known[key] > 0:
                known[key] -= 1
            else:
                introduced.append(finding)
        return introduced

# review_engine.py:400-410  (_line_free)
def _line_free(message: str) -> str:
    text = _LEADING_LINE.sub("", message)
    text = _PAREN_LINE.sub("", text)
    text = _BARE_LINE.sub("line", text)
    return " ".join(text.split())

# src/orchestrata_ultimate/refactor/codemod/report.py:93-102
class Finding:  # frozen dataclass
    severity: Literal["error", "warning"]; rule: str; path: str; line: int | None; message: str
    proof_tree: dict[str, Any] | None = None

# report.py:105-139
class CommitReport:  # frozen dataclass
    success: bool; files_written: tuple[str, ...]; findings: tuple[Finding, ...]
    duration_seconds: float; dry_run: bool; passes_run: tuple[str, ...] = ()
    diffstat: str = ""; correlation_id: str = ""
    def to_json(self) -> str: ...  # serialises the fields above

# src/orchestrata_ultimate/refactor/codemod/workspace.py  (commit, lines 59-109 of the method)
        all_findings: list[Finding] = []
        active = _resolve_reviews(reviews, review_engine, run_review_engine)
        if active:
            op.emit(CodemodEventType.REVIEW_STARTED, {"review_count": len(active)})
            for review in active:
                findings = review.review(self, changes)       # (try/except around it)
                all_findings.extend(findings)
                for finding in findings: op.emit(CodemodEventType.FINDING_EMITTED, {...})
            error_count = ...; warning_count = ...
            op.emit(CodemodEventType.REVIEW_COMPLETED, {"finding_count": ..., "error_count": ..., "warning_count": ...})
        error_findings = [f for f in all_findings if f.severity == "error"]
        if error_findings:
            op.emit(CodemodEventType.COMMIT_ABORTED, {...})
            return CommitReport(success=False, files_written=(), findings=tuple(all_findings), ...)

_source_lines(module, blank=frozenset()) (review_checks.py:118) returns the module's lines with readlines semantics; use it (or a thin _line_text(module, line) beside _line_free) to read the text of finding.line from the right tree.

6.2 The changes

B1. _delta(mutated, original, mutated_trees, original_trees): the key becomes (path, rule, _line_free(message), _line_text(tree_for_path, finding.line)) where _line_text returns the line stripped of surrounding whitespace, or "" when finding.line is None or out of range. Each side reads its OWN tree (mutated_trees[path] / original_trees[path]); findings on paths absent from changes.entries cannot occur (only changed modules are reviewed). Keep the multiplicity Counter. Update the _delta docstring and the module/engine docstrings. Tests in test_review_engine.py: (a) the reviewer's in-place case — API_KEY = «redacted» → API_KEY = "sk-" + 24 alphanumericsvia a realReplaceStringLiteralPass(or the CLI'sreplace-string) gives exactly one introduced error under the default; (b) a prepend above a legacy finding still gives 0 (line shift alone does not change the line's text); (c) a legacy line whose text changes only in whitespace gives 0 (strip); (d) the reviewer's B-mask shape — blank a legacy secret line and append a new secret elsewhere → exactly one introduced error. In tests/scripts/test_codemod_cli.py: the reviewer's CLI case (replace-string old=PLACEHOLDER new=sk-…) → rc 1 under the default, file unchanged with --apply`.

B2. New InMemoryReviewEngine.review_detailed(ws, changes) -> ReviewOutcome (frozen dataclass in review_engine.py: introduced: tuple[Finding, ...], legacy: tuple[Finding, ...]); review() becomes return list(self.review_detailed(ws, changes).introduced) so the ReviewPass protocol is unchanged. Under absolute, legacy == () and introduced is everything. In commit(): when a review in active is an InMemoryReviewEngine, call review_detailed, extend all_findings with introduced only (the veto set), collect legacy into legacy_findings, and emit each legacy finding as FINDING_EMITTED with an extra "legacy": True key; REVIEW_COMPLETED gains "legacy_count". CommitReport gains legacy_findings: tuple[Finding, ...] = () (after correlation_id, defaulted, additive) and to_json serialises it as "legacy_findings" with the same shape as findings. Both return CommitReport(...) sites in commit() pass it. Tests: the two tests that asserted an empty report (test_default_commit_writes_when_the_only_error_finding_is_legacy and tests/scripts/test_codemod_cli.py::test_run_default_baseline_is_delta_and_a_legacy_error_does_not_abort) now assert findings == () AND the legacy error present in legacy_findings (and in the JSON); absolute → legacy_findings == (); run_review_engine=False → legacy_findings == (). The event test in test_review_engine_commit_wiring.py (TST-110-026) asserts the legacy: True payload for a legacy finding. Record this as contract note D28 in the plan ledger cell ("CommitReport.legacy_findings additive field; engine review_detailed; orchestrator adds the contract JSON entry at ship-prep") — do NOT edit the contract JSON.

M1. review_engine.py module docstring (lines 12-14) and requirements.md:111 (REQ-110-035 row) say delta is the default (ADR-0073) and absolute the opt-in.

M2. tests/scripts/test_codemod_cli.py: test_run_default_baseline_aborts_when_the_pass_introduces_an_error — real temp repo, replace-string old=hello new=<sk- key> on a clean line → rc 1 dry-run and with --apply, file bytes unchanged, baseline: "delta" in the report.

C1 / C4 / records. Plan S3 ledger cell: replace the ROUND 5 whole-tree figures with the measured ones above (and say "measured by the round-6 reviewer at fc793fef2"); add "ROUND 6" with this round's measurements (6.3); add carried items C4 and the import os as x observation. Docs docs/codemod-review-engine.md: the delta paragraph states the key (path, rule, line-free message, stripped line text), the in-place shape it now catches, the remaining masking shape (removed legacy finding plus identical text AND identical line text elsewhere in the same file), and that legacy findings appear under legacy_findings without vetoing.

6.3 Expected measurements after round 6

6.4 Done when (round 6)

(1) B1 and B2 implemented as in 6.2 with their tests; (2) M1, M2 done; (3) every 6.3 row at its expected value, with the scripts-dry-run re-fire check reported; (4) §8 gates at expected results (ast-sync first for gate.py run), ruff/black/pyright clean, coverage ≥ the round-5 figures (review_engine.py 100%, workspace.py ≥ 91%, scripts/codemod.py ≥ 97%, report.py measured); (5) ledger, docs, D28 note written; RTM regenerated (unverified); (6) commits as Mohamed on top of fc793fef2, tree clean except hook litter; report with shas, the 6.3 table filled, pytest counts, deviations.

Round 7 — round-7 reviewer findings (orchestrator's picks, 2026-10-07)

Base: 7397f2c35 (round 6 landed in ac8188445, eb44e2a5e, 7397f2c35). The round-7 reviewer closed B1, B2, M1, M2 and the eight verification items, re-measured the whole tree under absolute at 235 errors in 93 files / 12,786 warnings and delta 0, and returned NO-GO on ONE blocker: a pre-existing S1 test that pins the exact REVIEW_COMPLETED payload and now sees the key round 6 added. Regression over all of tests/refactor/codemod/ plus tests/scripts/test_codemod_cli.py (reviewer's groups): 1,068 passed, 1 failed, 0 skipped; tests/verification/review/ 161 passed. Round 6's groups (93 / 71 / 20 / 55 / 161) never included test_observability.py; this round's done-when runs every module.

7.1 Exact source at 7397f2c35

Failing test (tests/refactor/codemod/test_observability.py:945-985, TestWorkspaceCommitObserverComposition.test_commit_event_sequence_and_payload_via_debug_panel_ndjson), the assertion at lines 969-973:

        assert lines[0]["payload"] == {"review_count": 1}
        assert lines[1]["payload"] == {
            "finding_count": 0,
            "error_count": 0,
            "warning_count": 0,
        }
        assert lines[2]["payload"] == {"file_count": 0, "dry_run": True}

Producer (src/orchestrata_ultimate/refactor/codemod/workspace.py, inside CodemodWorkspace.commit, the REVIEW_COMPLETED emit near line 712): the payload is {"finding_count": ..., "error_count": ..., "warning_count": ..., "legacy_count": len(legacy_findings)}. Read it with:

.venv/bin/python scripts/serialize_codebase_ast.py source refactor.codemod.workspace.CodemodWorkspace.commit --no-daemon

Failure text from the reviewer's run: Left contains 1 more item: {'legacy_count': 0}.

Spec events table, specs/110-codemod-engine/spec.md:2310-2311:

| `FINDING_EMITTED` | After a review pass produces a finding | `severity`, `rule`, `path`, `line`, `message` |
| `REVIEW_COMPLETED` | After all review passes complete | `finding_count`, `error_count`, `warning_count` |

7.2 The changes

• R7-B1 (blocker). In the assertion above add "legacy_count": 0, after "warning_count": 0,. Nothing else in the test changes. The producer is correct (D28, ADR-0073); the test pinned the pre-ADR payload.
• Spec table (records the same change where the payload is specified). Amend the two rows at spec.md:2310-2311: FINDING_EMITTED gains legacy (bool, true on a legacy finding under the default engine; ADR-0073), REVIEW_COMPLETED gains legacy_count (ADR-0073). Keep the table's column layout; append to the payload cell, do not restructure.
• R7-M1 (records). The S3 ledger's carried list names the tools-dry-run re-fire at schedule_cron_tool.py:1245 as "the carried C5 pass defect" (passes.py:779 span defect). The reviewer confirmed the cause is different: _sweep_docstrings_and_comments (passes.py:729-779, loop at 746-748) rewrites EVERY simple ast.Constant str/bytes literal by design (its docstring says "string literals AND comments"), against REQ-110-010, which limits DocstringSweepPass to docstrings and comments. Measured by the reviewer with {X: Y} over src/orchestrata_ultimate/tools: 12 of 23 changed lines are not docstrings and change behaviour (remote_trigger_tool.py:184/215/249/349 HTTP header "X-Signature" to "Y-Signature"; schedule_cron_tool.py:1172 env key SCHEDULE_CRON_EXECUTION_ID; schedule_cron_tool.py:1319 MAX_RETRIES_EXCEEDED; task_tools.py:201 enum value RATE_LIMIT_EXCEEDED; worktree_tools.py:690/782/783). Write this as its own carried item C6 ("the sweep's scope is all string literals, against REQ-110-010", high priority, owner S4/S5), and reword the C5 sentence in the carried list so it no longer attributes the tools re-fire to the span defect: C5 stays (span defect at passes.py:779), the re-fire is C6's symptom.
• Carried items to record (reviewer's C-a, C-b, C-c), not fixed in this round: C-a legacy findings are invisible in the CLI's human output (scripts/codemod.py:386-394 prints Findings: N errors, N warnings; _cmd_report prints "No findings."); they appear only in the JSON and the events (the scripts dry run carries 825, 33 of them errors). C-b an edit on a continuation line of a multi-line statement stays legacy because the finding sits on the statement's first line (y = eval(\n "1 + 1"\n) to y = eval(\n input()\n) gives introduced 0, legacy 1; the rule had already fired at that site). C-c server.py:385-410 RPC commit always commits an empty ChangeSet and does not serialise legacy_findings (predates S3). Add one sentence to docs/codemod-review-engine.md's delta paragraph stating C-b (the key is read from the finding's line, so a change on another line of the same statement does not make a finding introduced).
• Ledger ROUND 7 cell (same S3 cell, after ROUND 6): the reviewer's verdict (NO-GO on R7-B1 only; B1/B2/M1/M2 closed; eight items with their one-line verdicts; cost item: scripts dry run 28.6 s wall, 610 MB maxrss, 8.0 s reported, JSON 178 KB, GraphWriterObserver writes no node per finding; whole-tree absolute 235 / 93 / 12,786, load 26.1 s, review 79.3 s, 3,416 MB; delta 0; DoD: check_spec_authority rc 0, gate.py run rc 0 on an ast-synced archive, untraced-reqs clean for 033/034/035, coverage review_engine.py 100% / review_checks.py 99% / review_catalogue.py 95% / report.py 99% / scripts/codemod.py 97% / workspace.py 83% in the reviewer's run (98% with test_workspace_load.py), ruff/black clean on 16 files, check-deps rc 0; regression 1,068 / 1 / 0 + 161), then this round's fix with its shas and the full-suite counts. The reviewer's ship-blocker answer, verbatim in substance: S3 ships with a pass that corrupts non-docstring strings; that is REQ-110-010's defect, not S3's; the delta default removes the accidental veto on trees with legacy errors, so docstring-sweep --apply on the real tree is unsafe until S4/S5 fixes C6, high priority.

7.3 Done when (round 7)

(1) R7-B1 applied; (2) spec rows amended; (3) C6 written, C5 sentence reworded, C-a/C-b/C-c recorded, docs sentence for C-b added; (4) ledger ROUND 7 cell written; (5) the FULL regression, every module of tests/refactor/codemod/ in groups under the 300 s cap (the reviewer's runner is /tmp/claude-1001/-home-mohamed/42ea036c-e296-4b26-b447-2f612e9dd49a/scratchpad/r7rev/groups.sh; copy it to your own scratch dir and point S there), plus tests/scripts/test_codemod_cli.py and tests/verification/review/: expected 1,069 passed / 0 failed / 0 skipped + 161; (6) ruff/black clean on the changed files; check_spec_authority.py rc 0; (7) commits as Mohamed on top of 7397f2c35, no AI attribution, no "should", tree clean except hook litter (restore docs/CODEBASE_MAP.md docs/CODEBASE_MAP_FULL.md by name if a hook regenerates them); report with shas and the group counts.

Related

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