v0.9.0: the second review pass over the stack - #321
Open
samtalki wants to merge 15 commits into
Open
Conversation
GOCompetition publishes C3DataUtilities with no LICENSE file, so there is no grant to redistribute its validation data. cargo package was carrying tests/data/goc3_14bus_20220707.json into the powerio-prob sdist, which would have put a byte exact copy of that file on crates.io under powerio's own Apache-2.0/MIT terms. Excluding it keeps the fixture where the tests need it and out of everything downstream. The README claimed neither file shipped in a released artifact; that is now true, and it says which one is third party and why.
… gate The shims did not work where anyone would hit them. powerio/src/lib.rs re-exported only BalancedNetwork, so the deprecated alias was reachable at powerio::network::Network and nowhere else, and every existing use powerio::Network got a compile error instead of the warning the attribute promised. A deprecation that closes at 1.0 in two weeks is not a migration window anyway, so all nine deprecated items go and the crates take the break at 0.9.0 with the mapping in the CHANGELOG. PaperPure named neither the tool nor the formula and is gone entirely: the associated const, the serde alias, the CLI aliases, and the Python spellings. ReactanceOnly keeps its deprecation off. b = 1/x is the textbook DC linearization, and reproducing a published result needs it exactly as written, so removing it would have left nothing that does. The MCP server required powerio_version as a structural test for is this a .pio.json, so a package written by 0.8.x was not recognized as a package at all and the version rejection never fired. It now mirrors the Rust classifier, which keys on model_kind, and leaves the version gate to Package.from_json where the actionable message lives. envelope is retired as a word. It described the .pio.json top level in 40 places and explained nothing; the document is a package, and the error messages say so without defining it against what it is not.
…e source The header was last regenerated at #295, so every rename this stack made stopped at the Rust source: powerio.h still documented a `Network` type that no longer exists, and described the SCOPF writer by a wire schema constant that #318 replaced. The parity script only compares exported symbol names, so none of it failed CI. The star-lowering bullet was the reason regenerating was unsafe. It lived only in the generated header, not in cbindgen.toml where every other line of hand-written prose lives, so a regeneration would have silently deleted the one place the v4 bus-space trap is documented. It moves to cbindgen.toml and the header is generated again from one source.
Twelve review agents swept the change set from four angles; these are the findings that survived checking. Most of them are one rule written twice, which is how the numerical guards drifted mid-stack in the first place: ybus and the AC path each carried their own copy of the hypot bound, the denominator, the finite check, and the division, and #310 had to patch both in lockstep. That arithmetic moves to series_admittance_of, leaving each caller only its skip-vs-error policy. The same shape appears in the dense solvers, where dense_inverse and DenseCholesky::factor computed an identical pivot floor, and in the OPF builders, where limits.rs claimed the unrated-branch rule lived there once while the threshold and the branch stayed copied in dc.rs and ac.rs. powerio-pkg was flattening real errors: seven sites stringified a powerio::Error into Error::Payload, which reports category Data for everything, when the question mark operator and the crate's own From impl preserve the true category and print the same text. Error::Envelope is renamed Malformed, which the earlier vocabulary pass missed. The rest is stale text the stack left behind: the .pyi Convention still offered "paper", two doc comments still described a schema_version field that #318 renamed, and a duplicated assert.
Contributor
Conversion MatrixLegendCells show
Transmission7 cases.
Distribution7 cases.
Full warning details: download |
schemars carries doc comments into the schema descriptions, so correcting the two that still named schema_version and the package envelope moved the generated file with them.
The multiconductor table's Julia column still said `DistNetwork` in four of its rows while a fifth already said `MulticonductorNetwork`, so one table documented two names for one type — and the one it documented four times does not exist: PowerIO.jl exports `MulticonductorNetwork` (src/PowerIO.jl:62). The Rust column is correct as it stands; `DistTargetFormat` really is still the type name (powerio-dist/src/convert.rs:49), and renaming the remaining Dist* element structs is a 1.0 item. The balanced table said `Network::from_json`, which the flip renamed, and routed the C ABI column through `pio_parse_str` with a `"powerio-json"` token when `pio_from_json` has existed all along (powerio-capi/src/lib.rs:517). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tion Two defects an end-to-end advisory pass over the stack turned up, both in the class where a caller gets numbers instead of an error. The version gate contradicted the release it was written for. `supports` required the document's major to equal the build's, so a `.pio.json`, DC OPF bundle, or geo layer stamped `powerio_version: 0.9.0` would be refused by a 1.0.0 build. The whole premise of 0.9.0 is that its formats are what 1.0.0 publishes and that 1.0.0 removes deprecated items rather than changing what is written, so that gate would have made every consumer regenerate an archive whose bytes did not change, twice: once for 0.8 to 0.9 and again for 0.9 to 1.0. One lineage now crosses the major boundary and only one; 2.0 froze nothing and reads neither. The rejection message names it, so a caller holding a 0.9 document is not told to regenerate it. `supports` splits into a `reads` taking the build's own lineage as an argument, because the 1.x behavior cannot otherwise be tested from a 0.x build, and the previous attempt at that test asserted a copy of the branch rather than the branch. The DC convention default flipped from `PaperPure` to `SeriesImpedance` on three surfaces at once, and the successor of the old default is `reactance-only`, not the similarly named `series`. Nothing said so. A caller who passes the retired spelling now gets an error naming `reactance-only` and stating that `series` is a different formula; the CLI help says the same where a shell user would look. The Rust name is a compile error already, which is the one surface that was safe. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ler's document Three defects an end-to-end correctness pass over the stack turned up. The cost row artifact rule was stated twice and the two copies already disagreed. `GenCost::quadratic_with_constant_tol`, added by this stack, has no caller anywhere in the workspace; the OPF builders reach `nodal::quadratic_terms`, which rolls the rule again. They diverged in both directions: the hub compares the source coefficient against the tolerance while prob compared twice it, so the effective threshold differed by a factor of two, and a longer row whose leading coefficients are rounding artifacts read as quadratic through the hub while erroring as an unsupported model in the builders. The public API and the builders disagreed about which rows load. `quadratic_terms` now calls the hub method. `powerio-pkg` reported a caller's malformed document as a failure to serialize our own output. `set_fields` values are inserted untyped and only fail on the round trip back to the typed payload, and both round trip sites went through the blanket `From<serde_json::Error>`, whose own doc comment claims a caller's document can never reach it. A study edit setting a numeric field to a string surfaced as "serializing .pio.json: invalid type" with category Output, so a consumer branching on the category would retry a write instead of fixing the file, and the Python layer raised the wrong exception class. Both sites now name the payload, and a test pins the category rather than the message. `DegenerateTap` told every caller that Y_bus could not divide, including the ones that never build a Y_bus: incidence assembly and both OPF builders raise it too, and under `SeriesImpedance` the tap is not read at all. The string crosses the C ABI, so a Julia user running a DC build was told a matrix they never asked for had failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The doc comment on `supports` linked the private constant, and the build job documents the workspace under -D warnings, so rustdoc denied the private intra-doc link and the whole job failed while every test passed. The rule the sentence states is public; the constant holding it is not, so the sentence names the lineage instead of linking to it. ci-mirror.sh ran fmt, clippy, the header parity check and the tests, but never the doc build, which is why this reached CI. It runs it now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A redteam pass over the release found that the DSS executor caps include nesting depth and nothing else. Depth bounds one branch, not the work: a file that redirects to itself twice expands into a binary tree of depth 64, so 62 bytes of input never finish parsing. Reproduced on the shipped executor, the process was still running at 45 seconds with resident memory climbing about 35 MB/s and no plateau, and every leaf at the depth limit pushed another warning string onto a list nothing drains. It is reachable from every entry point that reads a file: the CLI, the Python wheel, `pio_dist_parse_file` across the C ABI, and the MCP server once the case sits inside an allowed root. `parse_dss_str` was never affected, because it disables includes outright, and that is exactly why the fuzz target missed this: `fuzz/fuzz_targets/dss.rs` exercises the string parser, so the include executor has no fuzz coverage at all. Two budgets now sit beside the depth check: the number of includes a parse may follow, and the script bytes they may pull in. Either one refuses the rest of the tree with a message naming both, through a diagnostic of its own rather than the containment code, since exhausting a budget is not an escape attempt. The root file is not charged, so a case without includes can never be truncated, and the largest fixture in the tree follows three includes against a budget of four thousand. Three release-mechanics gaps came out of the same pass. The schema job compared tracked files, so the new lineage directory a version bump creates would have been invisible and the job would have gone green while the document went uncommitted; it checks the working tree instead, in CI and in the local mirror. A tag whose name disagrees with the workspace version now fails the binaries build rather than producing a library that calls itself the previous release and stamps that into every document it writes. And a test pins the frozen lineage to be no older than the build's own, so a 0.x cut past it cannot quietly become a lineage no 1.x build reads. SECURITY.md now states the confinement model plainly: includes resolve inside the case file's own subtree and nothing inside that subtree is restricted, so an untrusted case belongs in a directory of its own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
synthesize_rate_a tested zmag == 0.0 while every other division in the stack routes through MIN_DIVISIBLE_MAGNITUDE, so a branch at zmag = 1e-200 returned ~1e200 as a thermal limit. It now uses the shared bound, and the two agree on which branch has no impedance to bound a current with. ReferenceBuses::single returned powerio::Result from the one crate whose error.rs declares crate::Result to be what every fallible entry point returns. The mechanical rename rewrote three doc comments into statements that are false about the code they sit on, including "BalancedNetwork level coordinate metadata" on the multiconductor model. Two powerio-prob tests still named the wire form the one-version collapse removed, and one asserted the opposite of what it was named. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every boolean flag was positional-or-keyword, so net.ybus(False, True) was legal and told a reader nothing. Inserting `*,` after 1.0 breaks every positional caller, so it happens now: lacpf, ybus_parts, ybus, to_normalized_with_options, write_gridfm and write_gridfm_batch take their options by keyword. The three exception classes declared __module__ = "_powerio", a module path that does not exist. pickle could not find them, so a parse failure raised in a ProcessPoolExecutor worker reached the caller as a PicklingError instead of something it could catch. They now name the module they are importable from, and the three pyclass handles say powerio._powerio rather than defaulting to builtins. ModelKind is non_exhaustive and ModelPayload was not, though the two are required to agree. Adding the attribute after 1.0 is itself a breaking change, so it goes in while it is still free. BalancedNetwork delegates its data attributes through __getattr__ with no __dir__, which left every one of them invisible to tab completion. The stubs undersold the surface: switches had no TypedDict at all, Branch was missing eleven fields the getter populates, and three working Package methods were undeclared. Typing as_multiconductor as something other than Any then caught a real type error in the MCP server, where net was bound to a MulticonductorNetwork and rebound to a BalancedNetwork. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The GeoMeta doc comment feeds the generated description, so restoring it to "Network level" on the multiconductor model changes the schema too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The keyword-only change landed in __init__.pyi and the Rust signatures but not in _powerio.pyi, so stubtest read twenty-one parameters as still positional. The MCP server also called write_gridfm positionally, which is the exact break the change exists to prevent, and it surfaced only on 3.12 because the mcp tests skip on 3.9. Switch joins the allowlist beside Bus and Branch: a TypedDict has no runtime object. switches and graph_json are declared rather than left to --ignore-missing-stub, which is what let the stubs drift from the runtime in the first place. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1bf7c63 removed all nine deprecated items and kept ReactanceOnly deliberately, because b = 1/x is the textbook DC linearization and reproducing a published result needs it exactly as written. The bundle page still promised removal in 1.0.0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Stacked on #320. This is the second review pass over the v0.9.0 stack, in the same role #316 played for the branches below it: a review found defects spread across several of the open PRs, and the envelope vocabulary change touches files that move in most of them, so folding the fixes back into their originating PRs would conflict repeatedly. They land here instead, ahead of the ABI v5 work.
Five things.
The deprecation shims did not work.
powerio/src/lib.rsre-exported onlyBalancedNetwork, so the alias #320 added was reachable atpowerio::network::Networkand nowhere else: every existinguse powerio::Network;got a compile error rather than the warning the attribute promised. Since the whole set was scheduled for deletion at 1.0 anyway, all nine deprecated items go now and 0.9.0 takes the break, with the mapping in the changelog.scripts/deprecated-inventory.shstays as the gate that keeps them from returning.PaperPureis gone andReactanceOnlyis not deprecated. The associated const, the serde alias, the CLI aliases, and the Python spellings all go.b = 1/xis the textbook DC linearization, so reproducing a published result needs it exactly as written; deprecating the variant would have left nothing that does.An unlicensed fixture was shipping.
cargo package -p powerio-prob --listcarriedtests/data/goc3_14bus_20220707.jsoninto the sdist. GOCompetition publishes C3DataUtilities with no LICENSE file, so there is no grant to redistribute it, and the crate is Apache-2.0/MIT. It is excluded from the published crate and stays for local tests.The MCP server could not see a pre-0.9 package.
_package_valuerequiredpowerio_versionas a structural test for "is this a.pio.json", so a package written by 0.8.x was not recognized as a package at all and the version rejection never fired. It now mirrors the Rust classifier, which keys onmodel_kind, and leaves the version gate toPackage.from_jsonwhere the actionable message lives. Regression test included.The C header was last regenerated at #295. None of this stack's renames reached it, so
powerio.hdocumented aNetworktype that no longer exists. The parity script only compares symbol names, so nothing failed. Regenerating was unsafe because the star-lowering bullet lived only in the generated file rather than incbindgen.tomlwith the rest of the hand-written prose; it moves to the source and the header is generated again.The last commit is a simplification pass. The notable one:
ybusand the AC path each carried their own copy of thehypotbound, the denominator, the finite check, and the division, which is why the numerical guards drifted mid-stack and had to be patched in lockstep. That arithmetic moves toseries_admittance_of, leaving each caller only its skip-versus-error policy. Same shape in the dense solvers and the two OPF builders.powerio-pkgalso stopped flattening real errors intoError::Payload, which reported categoryDatafor everything."envelope" is retired as a word for the
.pio.jsontop level.