chore: post-repositioning scan — architecture enforcement, CI, bare-install lane - #32
Merged
Conversation
…nstall lane The scan's outputs: - tests/test_architecture.py: ARCHITECTURE.md's hard rules as static ast checks — runtime lane never imports linopy/xarray at module level, the engine imports nothing outside its subpackage (lazy included), expansion holds no mutable module state, every ir.Expr/Pred node is consumed by the executor, and the doc's module map stays complete. - .github/workflows/ci.yml (first CI): a bare-install job that asserts linopy is absent and runs the native suite, plus a full [dev,oracle] matrix job with ruff and the differential tests. - Bare-install collection: conftest collect_ignore skips oracle test modules when linopy is missing — native suite (26 tests) runs green on a clean install. - Two real bugs the bare lane caught: validate_piecewise_data imported xarray unconditionally (now gated to convex blocks with an actionable [oracle] message), and tidy_sources' guarded xarray import loaded it needlessly in dev environments (now consults sys.modules — a DataArray argument implies the caller already imported it). The linopy-free subprocess test now asserts xarray-free after solve, not just import. - ARCHITECTURE.md: hard rules marked as enforced; new Composition section (topology is data, not structure; compose-then-build; port/flow surface deliberately unnamespaced) referencing #29/#30/#31. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds CI and pre-commit checks, documents architecture and component composition, reduces optional runtime imports, refines schema/parser validation, reformats relational execution code, and adds architecture-focused test coverage. ChangesDevelopment workflow and architecture
Language and execution
Validation coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
One-time formatting pass; no semantic changes. Previously excluded from feature commits to keep diffs reviewable, now required by the format-check gate this PR introduces. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
setup-uv + uv sync replace pip in both jobs: the bare job syncs core deps only (--no-dev --no-default-groups) before asserting linopy is absent; the oracle matrix job syncs [dev,oracle]. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…p, strict ruff Wired like fluxopt: .pre-commit-config.yaml (hygiene hooks + ruff/ ruff-format at v0.15.7, matching the pinned dev-group ruff), dev deps as a pinned [dependency-groups] table (pytest, ruff, mypy, pre-commit, plus the oracle deps), and CI with concurrency-cancel, timeouts, and setup-uv caching. pyproject reconciled after the manual paste: single [tool.ruff] / [tool.pytest] tables, fluxopt's strict rule set adopted (bugbear, simplify, type-checking, perflint, refurb, ...) with repo-appropriate per-file ignores (grammar-token names in where_parser, prints in scratch benchmarks); fluxopt-specific leftovers dropped (pyrefly, src/fluxopt ignores, -n auto). Repo reformatted to the new style (single quotes, 120 cols) and all new lint findings fixed — notably TC-rule moves to TYPE_CHECKING blocks and SIM115 open()s in tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
.github/workflows/ci.yml (2)
19-19: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDisable credential persistence on checkout.
Both
actions/checkout@v4steps leavepersist-credentialsat its defaulttrue, exposing the token to any subsequent step/process in the job. Setpersist-credentials: falsesince neither job needs to push.🔒 Proposed fix
- uses: actions/checkout@v4 + with: + persist-credentials: false(apply to both the
nativeandfulljob checkout steps)Also applies to: 38-38
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml at line 19, Update both checkout steps in the native and full jobs to set persist-credentials to false, preserving the existing actions/checkout@v4 configuration otherwise.Source: Linters/SAST tools
12-13: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd an explicit
permissions:block.No top-level
permissions:is set, so the defaultGITHUB_TOKENgets its broad default scope for both jobs. Add a minimalpermissions: contents: read(workflow- or job-level) to follow least privilege.🔒 Proposed fix
concurrency: group: ${{ github.workflow }}-${{ github.ref }} cancel-in-progress: true +permissions: + contents: read + jobs:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 12 - 13, Add an explicit least-privilege permissions block to the CI workflow, granting only contents read access for the native job (or the workflow globally). Keep the existing job behavior unchanged while ensuring no broader GITHUB_TOKEN permissions are inherited.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@linopy_yaml/lowering.py`:
- Around line 286-290: Update the fallback RelationalBuildError message in the
helper validation logic to include “shift” in the advertised supported-helper
list alongside “sum”, “group_sum”, and “roll”. Preserve the existing
unsupported-helper error structure and eager-backend guidance.
In `@tests/test_architecture.py`:
- Around line 22-31: Update _module_level_imports to traverse module-level
compound statements such as try and if blocks, collecting runtime imports nested
within them. Continue skipping TYPE_CHECKING branches and do not descend into
function or class bodies, while preserving detection of direct module-level
imports and root-package extraction.
- Around line 70-75: Update the ImportFrom validation in the architecture test
to reject any relative import with node.level > 1, adding the imported module to
bad. Preserve the existing forbidden-module and linopy_yaml.relational checks,
while allowing only level-one relative imports within the relational subpackage.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Line 19: Update both checkout steps in the native and full jobs to set
persist-credentials to false, preserving the existing actions/checkout@v4
configuration otherwise.
- Around line 12-13: Add an explicit least-privilege permissions block to the CI
workflow, granting only contents read access for the native job (or the workflow
globally). Keep the existing job behavior unchanged while ensuring no broader
GITHUB_TOKEN permissions are inherited.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 52526715-8cd4-4fb8-93e9-5f1cf6d9e07e
📒 Files selected for processing (49)
.github/workflows/ci.yml.pre-commit-config.yamlARCHITECTURE.mdCLAUDE.mdlinopy_yaml/__init__.pylinopy_yaml/_notes.pylinopy_yaml/_patch.pylinopy_yaml/accessor.pylinopy_yaml/api.pylinopy_yaml/builder.pylinopy_yaml/compat.pylinopy_yaml/expansion.pylinopy_yaml/expression_parser.pylinopy_yaml/helpers.pylinopy_yaml/loader.pylinopy_yaml/lowering.pylinopy_yaml/piecewise.pylinopy_yaml/relational/__init__.pylinopy_yaml/relational/executor.pylinopy_yaml/relational/ir.pylinopy_yaml/schema.pylinopy_yaml/validation.pylinopy_yaml/where_parser.pypyproject.tomlscratch/relational_spike/bench.pyscratch/relational_spike/check_equivalence.pyscratch/relational_spike/duckdb_spike.pyscratch/relational_spike/executor_bench.pyscratch/relational_spike/gen_data.pyscratch/relational_spike/linopy_baseline.pytests/conftest.pytests/test_accessor.pytests/test_api.pytests/test_architecture.pytests/test_dispatch.pytests/test_error_notes.pytests/test_expansion.pytests/test_group_sum.pytests/test_language_boundary.pytests/test_loader.pytests/test_lowering.pytests/test_milp.pytests/test_parser.pytests/test_piecewise_block.pytests/test_piecewise_convex.pytests/test_relational.pytests/test_roll.pytests/test_schema.pytests/test_validation.py
Comment on lines
286
to
290
| raise RelationalBuildError( | ||
| f"{context}: helper '{node.name}' is not supported by the relational " | ||
| f"backend (v0 supports 'sum', 'group_sum', and 'roll') — use the " | ||
| f"eager backend" | ||
| f'eager backend' | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Error message omits shift from the supported-helpers list.
node.name in ('roll', 'shift') is handled two branches above, but this fallback error only advertises 'sum', 'group_sum', and 'roll' as supported — shift is silently missing from the message users see for any unsupported helper name.
As per coding guidelines: "Perform all validation at load time and provide clear, actionable error messages."
🐛 Proposed fix
raise RelationalBuildError(
f"{context}: helper '{node.name}' is not supported by the relational "
- f"backend (v0 supports 'sum', 'group_sum', and 'roll') — use the "
+ f"backend (v0 supports 'sum', 'group_sum', 'roll', and 'shift') — use the "
f'eager backend'
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| raise RelationalBuildError( | |
| f"{context}: helper '{node.name}' is not supported by the relational " | |
| f"backend (v0 supports 'sum', 'group_sum', and 'roll') — use the " | |
| f"eager backend" | |
| f'eager backend' | |
| ) | |
| raise RelationalBuildError( | |
| f"{context}: helper '{node.name}' is not supported by the relational " | |
| f"backend (v0 supports 'sum', 'group_sum', 'roll', and 'shift') — use the " | |
| f'eager backend' | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@linopy_yaml/lowering.py` around lines 286 - 290, Update the fallback
RelationalBuildError message in the helper validation logic to include “shift”
in the advertised supported-helper list alongside “sum”, “group_sum”, and
“roll”. Preserve the existing unsupported-helper error structure and
eager-backend guidance.
Source: Coding guidelines
Comment on lines
+22
to
+31
| def _module_level_imports(path: Path) -> set[str]: | ||
| """Top-level (non-lazy, non-TYPE_CHECKING) imported root packages.""" | ||
| tree = ast.parse(path.read_text()) | ||
| found: set[str] = set() | ||
| for node in tree.body: # module level only — function bodies are lazy | ||
| if isinstance(node, ast.Import): | ||
| found.update(alias.name.split('.')[0] for alias in node.names) | ||
| elif isinstance(node, ast.ImportFrom) and node.module: | ||
| found.add(node.module.split('.')[0]) | ||
| return found |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Detect guarded module-level runtime imports.
tree.body misses imports inside module-level try/if blocks. For example, try: import linopy still loads eagerly when installed but passes this rule. Traverse module-scope compound statements while explicitly skipping TYPE_CHECKING branches and function/class bodies.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_architecture.py` around lines 22 - 31, Update
_module_level_imports to traverse module-level compound statements such as try
and if blocks, collecting runtime imports nested within them. Continue skipping
TYPE_CHECKING branches and do not descend into function or class bodies, while
preserving detection of direct module-level imports and root-package extraction.
Comment on lines
+70
to
+75
| elif isinstance(node, ast.ImportFrom) and node.module: | ||
| m = node.module | ||
| if m.split('.')[0] in FORBIDDEN_RUNTIME | {'yaml'} or ( | ||
| m.startswith('linopy_yaml') and not m.startswith('linopy_yaml.relational') | ||
| ): | ||
| bad.append(m) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject relative imports that escape the relational package.
For from ..schema import MathSchema, node.module is "schema" and node.level is 2, so this check permits an engine dependency outside linopy_yaml.relational. Treat ImportFrom nodes with level > 1 as offenders; only level-one relative imports belong to the relational subpackage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_architecture.py` around lines 70 - 75, Update the ImportFrom
validation in the architecture test to reject any relative import with
node.level > 1, adding the imported module to bad. Preserve the existing
forbidden-module and linopy_yaml.relational checks, while allowing only
level-one relative imports within the relational subpackage.
This was referenced Jul 24, 2026
Merged
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
1 participant
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.
The review pass after #26, as discussed. Three deliverables plus two bugs it caught:
1. ARCHITECTURE.md, enforced (
tests/test_architecture.py): each hard rule as a staticastcheck — runtime lane never imports linopy/xarray, engine subpackage is import-isolated (lazy imports included),expansion.pyholds no mutable module state (rule 5's registry ban), everyir.Expr/ir.Prednode is consumed by the executor (primitive-completeness drift alarm), and the doc's module map must mention every module.2. First CI (
.github/workflows/ci.yml): a bare-install job that asserts linopy is physically absent and runs the native suite (26 tests), plus a[dev,oracle]matrix job (3.11/3.13) with ruff and the full differential suite (140 tests).3. Bare-install collection:
conftest.collect_ignoreskips oracle test modules when linopy is missing, so a cleanpip install linopy-yaml && pytestworks.Bugs found by actually running the bare lane:
validate_piecewise_dataimported xarray unconditionally — every native solve of any model would crash on a bare install. Now gated toconvex:blocks with an actionable[oracle]message.tidy_sources' guarded xarray import silently loaded xarray in dev environments; now consultssys.modules(a DataArray argument implies the caller already imported xarray). The subprocess linopy-free test now also asserts xarray-free after solve — the assertion gap that let this slip.Docs: ARCHITECTURE.md gains the Composition section (topology is data, not structure; compose-then-build; the port/flow surface as the deliberately-shared coupling contract), referencing the newly filed #29 (namespacing), #30 (schema merge), #31 (bounds as expressions).
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation
Chores
Tests