docs: fix five stale claims, and cut what git already remembers - #386
Merged
Conversation
📝 WalkthroughWalkthroughThe pull request updates architecture, roadmap, and specification documentation. It clarifies processing stages, data binding, import boundaries, language responsibilities, API contracts, solver status, roadmap sequencing, and round-tripping scope. ChangesDocumentation contracts
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
A precision pass over SPEC, ARCHITECTURE and ROADMAP after the linopy gap review. Five things were wrong rather than merely long. **`fill` is not a keyword.** SPEC 5.1 named it as the one keyword whose key is fixed; the surface has been `edge=` since the three boundary policies collapsed into one kwarg, and 7 documents it that way. A reference page naming a keyword that does not parse is the worst kind of stale. **Four dead issue links, all closed as completed.** #78 shipped duals, so citing it for the reduced costs and slacks that did *not* ship pointed a reader at a closed issue for live work — in both SPEC 10 and ROADMAP 2b. #82 was superseded by #204 when the solver session absorbed it. #28 closed; solver breadth is #106. #3 closed; the live version of that question is #381. **math -> LaTeX is not planned, it shipped.** ROADMAP Track 3 listed the first AST consumer as future work while `to_latex` / `to_typst` / `to_markdown` are in `__all__` and ARCHITECTURE describes the module. Track 3 now says what is left. **Two counts.** The relational lane has five off-spine modules, not four, and `_notes.py` was filed inside the `relational/` block of the module map though it is one of the six flat modules the same file lists. Shortening follows CLAUDE.md rather than taste: historical "this used to work differently" notes belong in git. Gone are the typeset fence retelling, the `memory_limit` rule that used to exist, the lazy import that used to exist, and the degree/piecewise refactor narrative — about forty lines whose rules are kept and whose stories are in the log. ARCHITECTURE also stopped restating SPEC 8's binding rules and stating `check`'s pipeline four times; SPEC stopped restating the Arrow boundary that 8 and ARCHITECTURE both own. Net: ARCHITECTURE -41 lines, SPEC -3, ROADMAP +5 (Track 3 gained detail it was missing). ROADMAP now also points at the five issues filed off this review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
FBumann
force-pushed
the
docs/precision-pass
branch
from
July 31, 2026 09:28
437c6b0 to
0af2df9
Compare
FBumann
enabled auto-merge (squash)
July 31, 2026 09:29
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/ARCHITECTURE.md`:
- Around line 177-184: Update the architecture claim in the surrounding prose to
state that data enters at exactly one place per lane, reflecting the separate
native and linopy entry points. Keep the existing explanation of model checking
and the closed waist unchanged, unless needed to maintain grammatical
consistency.
In `@docs/ROADMAP.md`:
- Around line 135-143: Update the Item 11 status statement to replace “blocked
by nothing” with wording that acknowledges it has no language or Track 4 blocker
while still requiring LP-file bound-section support. Keep the existing
explanation of HiGHS, linopy, and the LP-file sink work consistent with that
narrower claim.
🪄 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: 19f69706-108a-4404-a23c-0fcb5a452e4a
📒 Files selected for processing (3)
docs/ARCHITECTURE.mddocs/ROADMAP.mddocs/SPEC.md
Comment on lines
+177
to
+184
| That claim is enforced twice, because a renderer that imports only `language/` | ||
| still pays for polars if some language module does: a path-scoped import rule | ||
| like the other three fences, plus a check on the **transitive** closure. Two | ||
| properties carry the rest — **data enters at exactly one place**, which is why | ||
| checking a model costs seconds and needs nothing but the file, and the waist is | ||
| **closed**, which is what the ceiling in | ||
| [docs/design/ceiling.md](design/ceiling.md) protects: a new consumer is free, a | ||
| new primitive is taxed. What is planned, and why, is |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Qualify the single data-entry claim by lane.
Line [180] states that data enters at exactly one place. Earlier sections define two entry points: sources.py for native builds and linopy/loader.py for the linopy shim. This makes the architecture contract internally inconsistent.
Change the claim to “data enters at exactly one place per lane,” or document the intended abstraction explicitly.
Proposed wording
- data enters at exactly one place
+ data enters at exactly one place per lane📝 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
| That claim is enforced twice, because a renderer that imports only `language/` | |
| still pays for polars if some language module does: a path-scoped import rule | |
| like the other three fences, plus a check on the **transitive** closure. Two | |
| properties carry the rest — **data enters at exactly one place**, which is why | |
| checking a model costs seconds and needs nothing but the file, and the waist is | |
| **closed**, which is what the ceiling in | |
| [docs/design/ceiling.md](design/ceiling.md) protects: a new consumer is free, a | |
| new primitive is taxed. What is planned, and why, is | |
| That claim is enforced twice, because a renderer that imports only `language/` | |
| still pays for polars if some language module does: a path-scoped import rule | |
| like the other three fences, plus a check on the **transitive** closure. Two | |
| properties carry the rest — **data enters at exactly one place per lane**, which is why | |
| checking a model costs seconds and needs nothing but the file, and the waist is | |
| **closed**, which is what the ceiling in | |
| [docs/design/ceiling.md](design/ceiling.md) protects: a new consumer is free, a | |
| new primitive is taxed. What is planned, and why, is |
🤖 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 `@docs/ARCHITECTURE.md` around lines 177 - 184, Update the architecture claim
in the surrounding prose to state that data enters at exactly one place per
lane, reflecting the separate native and linopy entry points. Keep the existing
explanation of model checking and the closed waist unchanged, unless needed to
maintain grammatical consistency.
Comment on lines
135
to
+143
| Item 11 is the cheapest thing on this table and the only one blocked by nothing: | ||
| HiGHS carries `kSemiContinuous` and `kSemiInteger` natively, so unlike SOS | ||
| (#23) and quadratic it needs no Track 4 finding on the default sink; linopy has | ||
| `Model.semi_continuous`, so the oracle already covers it; and it adds a | ||
| declaration flag, not a plan node — no expression grammar, no lowering case | ||
| beyond the column type. The LP-file sink needs its own bound section, which is | ||
| where the work actually is. | ||
| where the work actually is — and which | ||
| [ARCHITECTURE](ARCHITECTURE.md#the-relational-lane) already anticipates as a | ||
| threshold on the `cols` stream. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Narrow the “blocked by nothing” claim.
If lp_file remains in the supported sink scope, item 11 has no language or Track 4 blocker, but it still needs LP-file bound-section support. Replace “blocked by nothing” with “has no language or Track 4 blocker,” or include the remaining sink work in the status statement.
Proposed wording
-Item 11 is the cheapest thing on this table and the only one blocked by nothing:
+Item 11 is the cheapest thing on this table and has no language or Track 4 blocker:📝 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
| Item 11 is the cheapest thing on this table and the only one blocked by nothing: | |
| HiGHS carries `kSemiContinuous` and `kSemiInteger` natively, so unlike SOS | |
| (#23) and quadratic it needs no Track 4 finding on the default sink; linopy has | |
| `Model.semi_continuous`, so the oracle already covers it; and it adds a | |
| declaration flag, not a plan node — no expression grammar, no lowering case | |
| beyond the column type. The LP-file sink needs its own bound section, which is | |
| where the work actually is. | |
| where the work actually is — and which | |
| [ARCHITECTURE](ARCHITECTURE.md#the-relational-lane) already anticipates as a | |
| threshold on the `cols` stream. | |
| Item 11 is the cheapest thing on this table and has no language or Track 4 blocker: | |
| HiGHS carries `kSemiContinuous` and `kSemiInteger` natively, so unlike SOS | |
| (`#23`) and quadratic it needs no Track 4 finding on the default sink; linopy has | |
| `Model.semi_continuous`, so the oracle already covers it; and it adds a | |
| declaration flag, not a plan node — no expression grammar, no lowering case | |
| beyond the column type. The LP-file sink needs its own bound section, which is | |
| where the work actually is — and which | |
| [ARCHITECTURE](ARCHITECTURE.md#the-relational-lane) already anticipates as a | |
| threshold on the `cols` stream. |
🤖 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 `@docs/ROADMAP.md` around lines 135 - 143, Update the Item 11 status statement
to replace “blocked by nothing” with wording that acknowledges it has no
language or Track 4 blocker while still requiring LP-file bound-section support.
Keep the existing explanation of HiGHS, linopy, and the LP-file sink work
consistent with that narrower claim.
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.
A precision pass over SPEC, ARCHITECTURE and ROADMAP. I went looking for prose to trim and found five things that were wrong, which turned out to be the more valuable half.
Wrong, not just long
fillis not a keyword. SPEC §5.1 named it as "the one keyword whose key is fixed rather than naming a dimension". The surface has beenedge=since the three boundary policies collapsed into one kwarg — §7 documents it correctly, §5.1 did not. A reference page naming a keyword that does not parse is the worst kind of stale, because it reads as authoritative.Four dead issue links, every one closed as completed:
math → LaTeX is not planned — it shipped. ROADMAP Track 3 listed the first AST consumer as future work, while
to_latex/to_typst/to_markdownare in__all__and ARCHITECTURE has a module-map entry and a paragraph about it. Track 3 now states what actually remains (#35, #34, #126, #321).Two counts. The relational lane has five off-spine modules, not four (
plan,frames,status,chunking,data_validation). And_notes.pysat inside therelational/block of the module map, though the same file correctly lists it among the six flat modules.Shorter, on the project's own rule
CLAUDE.md says historical "this used to work differently" notes belong in git. Applying that rather than taste, ARCHITECTURE loses ~40 lines of retelling while keeping every rule:
typeset/fence that was once untestedmemory_limithard rule that used to existpiecewiserefactor narrative (~35 lines) — the test it produced stays, stated once and forward-lookingLess duplication across files
check's pipeline four times (prose, diagram, prose, table) → diagram and table keep itAlso
ROADMAP now points at the five issues filed off the linopy review (#380–#384) instead of describing them as unfiled, and the degree section no longer states the MIQP blocker twice — an overlap introduced in #378.
Net: ARCHITECTURE −41, SPEC −3, ROADMAP +5 (Track 3 gained the detail it was missing). Full suite green — 709 passed,
mkdocs build --strictclean,tests/test_architecture.pyandtests/test_docs_site.pypass.Not done, deliberately: SPEC §10 is a Python API reference living in a page whose own header says it documents "what a YAML file may contain and what it means", while ARCHITECTURE — a design note under the nav — carries the pinned surface table. That is a real organisational seam, but moving a section between a Reference page and a Design note is a bigger call than a precision pass should make unilaterally. Happy to do it as its own PR if you want it.
🤖 Generated with Claude Code
Summary by CodeRabbit
shiftboundary semantics and clarified streaming API types.