fix(mirror): carry the tests into the exec mirror, and drop a layout-bound assertion - #92
Merged
Merged
Conversation
…nd one assertion was layout-bound TWO REGRESSIONS FROM #90, both found by running verify FROM THE MIRROR rather than reasoning about it. That is exactly what CLAUDE.md 1 requires and exactly what it warns of: "cmp-clean is not agreement, since a path resolved relative to a module's own directory is right in one tree and wrong in the other." 1. THE TESTS STOPPED TRAVELLING. Before the move the modules and the 33 test files were all root-level `.py`, so `orch-sync-mirror.sh`'s single `cp "$SRC"/*.py` carried BOTH. Afterwards `cp "$MODSRC"/*.py` carries only the modules, and a mirror run collected ZERO tests while reporting `floor 442 -- NOT MET`, which reads as a broken change rather than a broken sync. The sync now copies `$SRC/tests/*.py` and FAILS LOUDLY on either half being empty, so a hollow mirror can never be handed to launchd quietly. `pyproject.toml` gains `pythonpath = ["src", "."]`, and both entries are load-bearing in different trees: a checkout resolves the modules through `src`, the FLAT mirror through `.`. pytest ignores a pythonpath entry that is not present, so one list is right in both shapes — the same detect-don't-assume rule `paths.py` applies in Python and `orchestrate.sh` in shell. The sync also now REMOVES the file pyproject.toml replaced. Its delete step only clears `*.py` and `*.sh`, so the migrated-away `.coveragerc` lingered in the mirror, and `test_coverage_config_enables_parallel_mode` correctly refuses that: CI reads only pyproject.toml, so the two would disagree with nothing to say so. 2. AN ASSERTION I ADDED IN #90 WAS TRUE IN ONE TREE ONLY. `env_prereq`'s selftest asserted that a MODULE name is absent from the checkout root — true under `src/`, FALSE in the flat mirror where the module dir and the checkout root are the same directory. It passed in the repo and failed in the mirror. Removed rather than made conditional: the detector's real contract is layout-independent (present => None, absent => a reason naming it) and that is what the surrounding assertions already check. A test that only holds in one layout is worse than none, because it makes the OTHER layout look broken. Verified in BOTH trees, which is the point: * repo (src/): 442 passed, 0 failed, 0 skipped, 85/85 selftests, 5/5 gates. * mirror (flat): 419 passed, 0 failed, 23/26 skipped (the documented repo-config skips), 442 collected against floor 442, 85/85 selftests, 5/5 gates. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 47 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 74 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
Contributor
Workflow source neededPR #92 needs either a linked GitHub issue or one valid non-issue Workflow Source before PR metadata automation can manage it safely. Please do one of:
Once a valid source is present, this warning will not be reposted. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two regressions from #90, both found by running verify from the mirror rather than reasoning about it — which is what
CLAUDE.md§1 requires, and exactly what it warns of: "cmp-clean is not agreement, since a path resolved relative to a module's own directory is right in one tree and wrong in the other."1. The exec mirror lost its test suite
Before the move the modules and the 33 test files were all root-level
.py, soorch-sync-mirror.sh's singlecp "$SRC"/*.pycarried both. Afterwardscp "$MODSRC"/*.pycarries only the modules — a mirror run collected zero tests and reportedfloor 442 — NOT MET, which reads as a broken change rather than a broken sync.The sync script (outside the repo, patched separately) now also copies
$SRC/tests/*.pyand fails loudly if either half is empty, so a hollow mirror can never be handed to launchd quietly.In this repo:
pyproject.tomlgainspythonpath = ["src", "."]. Both entries are load-bearing, in different trees — a checkout resolves the modules throughsrc, the flat mirror through.. pytest ignores a pythonpath entry that doesn't exist, so one list is correct in both shapes; same detect-don't-assume rulepaths.pyapplies in Python andorchestrate.shin shell.The sync also now removes the file
pyproject.tomlreplaced. Its delete step only clears*.pyand*.sh, so the migrated-away.coveragerclingered — andtest_coverage_config_enables_parallel_modecorrectly refuses that, because CI reads onlypyproject.tomland the two would disagree with nothing to say so.2. An assertion I added in #90 was true in one tree only
env_prereq's selftest asserted that a module name is absent from the checkout root. True undersrc/; false in the flat mirror, where the module dir and the checkout root are the same directory. It passed in the repo and failed in the mirror.Removed rather than made conditional. The detector's real contract is layout-independent — present ⇒
None, absent ⇒ a reason naming it — and the surrounding assertions already check exactly that. A test that holds in only one layout is worse than none, because it makes the other layout look broken.Verified in both trees, which is the point
src/)🤖 Generated with Claude Code