engine: audit and repair diagram syncs after model edits - #1067
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04e39c0e29
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| diff_clouds(&mut state, &metadata); | ||
| declutter::declutter_part(&mut state.elements, needs_label_placement, |_| false); | ||
| apply_loop_curvature(&mut state, &config, model, &metadata); | ||
| apply_loop_curvature(&mut state, &config, model, &metadata, created_link); |
There was a problem hiding this comment.
When a patch only changes an existing flow's attachment—for example, deleting its destination stock—retarget_flow can move the flow's valve while new_elements remains empty. This branch then returns without calling rebow_moved_links, so curved connectors ending at that valve retain an absolute takeoff angle for the old chord and can visibly change curvature or render nearly straight. Run rebow_moved_links here as well before applying curvature and returning.
AGENTS.md reference: src/simlin-engine/AGENTS.md:L191-L191
Useful? React with 👍 / 👎.
| if let Some(n) = after.get_name() | ||
| && named_ident(after).as_deref() == Some(image) | ||
| { | ||
| set_name(&mut expect, n); |
There was a problem hiding this comment.
For every named element, image is also the element's unchanged canonical identifier when the patch contains no rename, so this block accepts any new spelling with the same canonical form. An unrelated sync that changes Birth Rate to birth_rate or rewrites its stored \n wrapping is therefore treated as unchanged and never raises UntouchedElementChanged; only substitute the post-edit name when this element was actually renamed, and otherwise compare the original element exactly.
AGENTS.md reference: src/simlin-engine/AGENTS.md:L193-L193
Useful? React with 👍 / 👎.
| UpsertFlow(UpsertFlowInput), | ||
| UpsertAuxiliary(UpsertAuxiliaryInput), | ||
| RemoveVariable(RemoveVariableInput), | ||
| RenameVariable(RenameVariableInput), |
There was a problem hiding this comment.
When renameVariable is used on a variable that already owns an error, such as an unresolved reference, the same diagnostic remains after the engine rename but its formatted variable_name changes from from to to. The validation gate compares post-edit (code, variable_name) tuples against unmodified pre-edit tuples, so it classifies that existing error as new and rejects the rename, contrary to EditModel's existing-error tolerance; normalize the pre-edit keys through the requested renames before computing the diagnostic delta.
Useful? React with 👍 / 👎.
| .map(|f| (f.kind, f.subject)) | ||
| .collect(); | ||
| for finding in consistency_findings(&after, &changed, &identity) { | ||
| if !before_keys.contains(&(finding.kind, finding.subject.clone())) { |
There was a problem hiding this comment.
The baseline subtraction keys findings only by (kind, subject), discarding the detail and multiplicity. On an imported view that already has one dangling reference for a flow, for example, a sync can introduce a second dangling attachment—or replace it with a different dangling UID—and every post-edit DanglingReference for that flow is suppressed because the coarse key existed before. Compare finding multisets using enough identity to distinguish individual defects so the audit actually charges inconsistencies introduced by the edit.
AGENTS.md reference: src/simlin-engine/AGENTS.md:L193-L193
Useful? React with 👍 / 👎.
| let steps = ((length - 2.0 * VALVE_CLAMP_MARGIN) / STEP).floor() as usize; | ||
| let best = (0..=steps) | ||
| .map(|i| VALVE_CLAMP_MARGIN + i as f64 * STEP) | ||
| .filter(|&s| clear(point_at_arc(&f.points, s))) | ||
| .min_by(|a, b| (a - at).abs().total_cmp(&(b - at).abs())); |
There was a problem hiding this comment.
When a newly created flow's initial valve overlaps another shape, this scans one candidate per pixel along the entire pipe, and each candidate walks both the polyline and every shape. Existing diagram coordinates are input data, so a flow between stocks separated by millions or billions of units can make a single MCP or libsimlin sync perform millions or billions of iterations, effectively hanging it. Compute blocked arc-length intervals, search outward from the current valve, or otherwise cap the candidate count instead of scaling directly with coordinate distance.
Useful? React with 👍 / 👎.
| UpsertFlow(UpsertFlowInput), | ||
| UpsertAuxiliary(UpsertAuxiliaryInput), | ||
| RemoveVariable(RemoveVariableInput), | ||
| RenameVariable(RenameVariableInput), |
There was a problem hiding this comment.
The new MCP rename operation also accepts renames whose canonical identifier is unchanged, such as students to Students. The engine correctly restamps the model's display spelling, but incremental layout looks up the destination's existing display_names entry and therefore keeps students on the view; the saved model and diagram disagree about the requested label. Ensure the rename sync uses the caller's exact to spelling for case-only renames as well.
Useful? React with 👍 / 👎.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1067 +/- ##
==========================================
- Coverage 93.34% 93.29% -0.05%
==========================================
Files 280 282 +2
Lines 163198 165608 +2410
==========================================
+ Hits 152335 154508 +2173
- Misses 10863 11100 +237 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Code reviewReviewed the diff (16 commits, ~5860 additions across No P0/P1 findings. The determinism fixes (surviving links/clouds retained in place, new ones appended in Two smaller notes (both P3, both pre-existing but relevant because this PR exposes the rename path to LLM callers over MCP):
Neither is introduced by this PR, so not blocking; flagging because the MCP addition changes who can trip them. Overall correctness: correct. The audit's decision-table tests and the empty 🤖 Generated with Claude Code |
A diagram sync re-listed every link and cloud: diff_connectors and diff_clouds removed them all and pushed the survivors back, links sorted by uid pair and clouds in HashSet order. Restating a variable exactly as it was therefore reordered the view's elements, and two syncs of one edit could list them differently. The element list is the view's draw order and what a saved file lists, so every MCP edit churned the file even when the diagram did not change. Surviving links and clouds now stay where they are and created ones are appended in uid order. A link survives when the dependency it draws, read through aliases, still exists: the set the old consume-and-re-push passes kept, except that two links drawing one exact pair are both kept rather than collapsed to one.
Diagrams generated from scratch are measured by the layout-quality metric and its harness, but a diagram an agent edits is synced by incremental_layout after every patch, and a static score cannot say whether a sync did the right thing: whether it left alone what the edit did not touch, kept the view consistent with the model, and put what it created somewhere sensible. The only evaluation of that path was a replay that adds chains, which never deletes, re-attaches, renames or changes a variable's kind. layout::edit_audit states the contract from an edit's inputs and outputs alone, without reading how the sync works: scope (untouched elements and surviving links come back as they were), consistency (the view agrees with the model and routed flows hold the strict flow invariants, charging only what the edit introduced, so an imported view's own inconsistencies are not the edit's), and placement (created elements land on no shape, routed pipes through no foreign stock). layout::edit_scenarios generates sixteen kinds of edit for any model, picking targets deterministically, and drives them through apply_patch and the production sync rule, checking that two syncs of one edit agree and that an undo returns the original view. The battery pins every finding the sync still produces in KNOWN_DEFECTS, one row per fixture, scenario and finding naming its defect: a new finding fails, and so does a row that stops reproducing, so each fix deletes its rows. The audit's decision table has one row per finding kind, each checked on a correct and a defective view built from a synced edit. The layout_eval harness runs the same scenarios over the corpus and writes edits.html with marked before and after renders. The connectors a sync draws for dependencies an author left out are recorded but not charged by the audit; the SIR rows that come from them wait on a product decision about whether a sync should complete a view.
apply_loop_curvature curves every straight link on a detected feedback loop, and incremental layout ran it after every edit, so an equation-only edit, or one that closed a new loop, re-curved links a person had drawn straight. The edit battery found it on every scenario over a view with straightened links. The pass now takes a filter: a fresh layout curves every loop link, and an incremental pass only the links it creates, recognized by a uid the view before the edit did not use. The battery rows it produced are gone.
A stock an edit hangs off a drawn chain takes the chain's next spot and fans vertically only past what is already there, but the check counted stocks, parameters and modules only. A side flow drawn off the face the chain continues from put its valve or cloud on that spot, and the new stock landed on it: the battery found it on fishbanks, whose harvest pipe runs right to a cloud where a downstream stock would go. Valves, clouds and aliases now block the spot as well.
When an edit changed a variable's kind, or the stocks a flow attaches to, incremental layout removed the element with apply_deletion, which also drops every link and alias touching it, and rebuilt it as a new element placed afresh. An agent turning a parameter into a stock saw it jump across the diagram (127 to 447 px on the battery's fixtures), and every link into a re-attached flow came back re-created, losing the curve a person gave it. LayoutState::remove_for_rebuild takes out only the element and its clouds, keeping the uid, position and display name, so the rebuilt element takes the same uid and the links and aliases survive; the connector diff still drops a link whose dependency is gone. A variable that became a stock, aux or module is redrawn at its old center, and the polish passes do not move it; its label side is still chosen afresh. A variable that became a flow is placed as a new flow, since its valve belongs on the pipe between its stocks. A surviving curved link whose endpoint moved turns its takeoff with its chord, so it keeps its bow.
When an edit changed the stocks a drawn flow attaches to -- a stock deleted, a flow dropped from a stock's list, a flow listed on another stock -- incremental layout threw the flow away and laid it out again as a new flow. Deleting a chain's middle stock moved the flows through it and piled their clouds onto valves, and restating a stock without one of its flows put the new cloud inside that stock and ran the pipe through it. retarget_flow moves only the ends that changed, through the editing core's own flow geometry (heal, route_end, route, now re-exported crate-wide), so a tool edit's pipes are drawn by the same code as a touch edit's. An end that becomes a cloud stays where it was, moved along its pipe past the face of a stock it no longer attaches to; an end that attaches to another stock is routed to it, keeping as much of the pipe as stays valid and routing around stock bodies. The flow keeps its uid, name and valve, unless a new cloud would cover the valve, which then takes the middle of the pipe; its label side is chosen again. A flow that must attach to a stock this pass creates has no drawn stock to route to and is still rebuilt as new.
A flow an edit creates is laid out by the finishing pass, which orthogonalizes between its two ends and sees no other stock, so a flow between two stocks with a third drawn between them ran straight through the third and read as attached to it (SIR, recovered to susceptible past infectious). Placement also put a created valve where a person had parked something else: a parameter, another flow's valve. A created flow whose pipe passes through a stock that is not one of its ends is now re-routed by the editing core's router, which ranks a path through a stock as crossing and generates detours around it; a pipe that already clears every stock keeps what the finishing pass gave it. A created valve that covers another shape slides along its pipe to the nearest position clear of every shape, keeping VALVE_CLAMP_MARGIN from the pipe's ends. The detours clear shapes but not labels or links: SIR's bypass runs over the stock names above the chain. The audit charges shapes only, so that is for the placement-quality layer of the eval to measure.
A diagram sync drew a link for every dependency the view did not draw, whatever the edit was: any edit to SIR drew total_population -> susceptible, a connector the author had left out, so a view kept changing in parts no edit touched. The decision is that a sync must not change parts of the diagram unrelated to the edit. diff_connectors now draws a missing link only when a filter accepts its dependency. Incremental layout accepts a dependency into a variable the patch names (upserted, renamed, or re-listed), or one whose end is drawn for the first time, since a new element carries no author's choice about its connectors; fresh layouts draw everything. The audit charges the rest as UnrelatedLinkAdded instead of recording it as an uncharged note, and the battery rows that waited on the decision are gone.
An agent without a rename operation renamed a variable by removing it, upserting it under the new name, and rewriting every equation that read it. The diagram sync saw a deletion and a creation, so the variable's element was drawn somewhere new with re-created links (on the population default project the layout cost rose from 0.31 to 0.54), and a reader the agent missed either failed the new-error gate or left a stale reference. renameVariable maps to the engine's RenameVariable, which rewrites every reference atomically (equations, stock flow lists, module references, group members), and the sync keeps the element's uid, position and links. UpdateStockFlows stays off the surface: upsertStock already sets a stock's flows, and a second way to say that would make a patch holding both order-dependent and blur upsert's create-or-replace rule.
The corpus run found syncs on imported Vensim views removing an author's link the dependency extraction does not explain, and drawing variables the author never drew, on edits about neither. The audit let both pass: a link without a recognized dependency counted as stale whatever the edit named, and a variable drawn for the first time was an uncharged note. Under the decision that a sync must not change parts of the diagram unrelated to the edit, both are defects. A link drawing no dependency now survives unless the patch names its reader, and a variable the view did not draw may be drawn only when the patch names it (UnrelatedElementAdded replaces the note). The battery gains the imported fixtures whose shapes the corpus run exposed (groupon, catastrophe, beer game, bathtub, alias1), pinning every finding under the defect behind it; the scenario targets read dependency metadata without loop detection, which keeps the larger fixtures within the unit-test budget.
XMILE spells a module input's source in the enclosing model as `.x`, which the reader stores as `·x` and every consumer reads as the bare name. A rename rewrote only `self·x`, so renaming the variable left the module reading a name that no longer exists: the input port silently took its default and the connector into the module was dropped. The edit battery found it renaming `area` in hares and foxes. The rename rule now treats the empty qualifier like `self`, and keeps the reference's own spelling, as it already did for `self·x`.
A sync pinned the label sides of the elements that survived it, but not of aliases, so the declutter chose every alias's side again on every edit: on imported Vensim views, which draw shadow copies of variables, any edit churned labels the author had placed. A sync never creates or rebuilds an alias, so every alias now keeps its side. Pinning them unmasked a placement defect: in a crowded region the declutter's relaxation can jam, and a created parameter stays on another shape (catastrophe's inserted intermediate over an alias, which the re-chosen alias label had happened to clear). It is pinned in the battery for the placement fixes.
The connector diff removed every link whose dependency the extraction does not recognize, whatever the edit was about: on groupon's imported view any edit dropped repeaters -> subscribers, and undoing the edit could not bring it back. Such a link is an author's choice the model does not explain (a port the walk does not see, an annotation), and a sync must not change parts of the diagram the edit is not about. diff_connectors now keeps a link drawing no dependency unless a filter refuses its reader; incremental layout refuses a variable the patch names, whose equation says which connectors into it belong, and fresh layouts refuse everything. Keeping those links exposed that deleting a variable removed its aliases but not the links touching them, which the connector diff had been cleaning up by accident; the deletion now removes them itself. The combined-operations integration test built its patched model by hand, changing a flow's equation without the patch naming the flow, an input production never supplies. It now derives the model from the patch through apply_patch, on a model carrying the synced view.
A sync drew every variable the view had no element for, whatever the edit was, because completeness was checked against the whole model: on imported Vensim views, where an author leaves helpers and synthesized net flows out of a sketch, any edit drew them (catastrophe's five auxes, beer game's four net flows, whose clouds then landed on stocks). A sync must not change parts of the diagram unrelated to the edit. Incremental layout now draws a variable the view does not draw only when the patch names it, keeps drawing everything the view drew, and checks completeness against that set; a fresh layout still draws and checks every variable. The connector test's arm for an element drawn for the first time now draws it through an edit that names it.
Two ways a sync put a cloud on something already drawn. Deleting a stock turns the ends of the flows through it into clouds where the ends were, on the stock's faces, and on two perpendicular faces those clouds cover each other (bathtub, catastrophe). And a created side flow's cloud was kept off other clouds only, so it landed on a stock a person had parked where the pipe ends (bathtub's measured stock). A re-attached flow's new cloud end now slides back along its own pipe, a cloud's radius at a time, to the first spot covering no shape but the flow's own; a created flow's free end is pushed out along its pipe past shapes as well as clouds.
The declutter's relaxation pushes footprints apart, but in a crowded region it can jam and return without clearing them, and incremental layout used what it returned: catastrophe's inserted intermediate stayed on an alias, an overlap the alias's re-chosen label had happened to clear before aliases kept their label sides. A created parameter or module whose shape still covers another shape now moves to the nearest clear spot, searched on rings around where it landed. Nothing drawn before the sync moves, so the created element takes the clearance. The battery's known defects are empty again.
A cloud end a deleted stock leaves behind slid back along its end segment only, and an imported view can draw two flows leaving one face point along one line: thyroid's plasma T4 sends k21 and k31 up together for 37 px before they turn apart, so after deleting it their clouds sat exactly on top of each other, as did k12's and k13's. The end now slides along the whole pipe toward the valve, a cloud's radius at a time, stopping short of the valve, and the pipe drops the points the end passes.
A scenario picked the first candidate by ident whether or not the view drew it, and on industrial dynamics that was iad_net_flow, a net flow the author left out of the sketch. Restating it names it, and a sync draws a variable an edit names, so the restate and the add-then-undo scenarios reported that the view did not return to the original: a false finding about the scenario, not the sync. Targets now come from the variables the view draws (readers included), while fresh names still avoid every variable. Drawing an undrawn variable an edit names is pinned by incremental layout's own tests.
The corpus run's large imported models turned up two more ways a sync put something on a shape already drawn. When a re-attached flow's new cloud covers its valve, the valve takes the pipe's middle, and on industrial dynamics that was an alias; the valve clearance only ran on flows the sync created. And a parameter turned into a stock is redrawn at its old center, where a stock's larger body covered thyroid's neighbouring parameter. Re-attached flows now get the valve clearance created flows get. A variable redrawn in place moves to the nearest clear spot when its new shape covers another shape there, unless a flow attaches to it. The audit charges a rebuilt element's move only where its new shape at the old center would have covered nothing, since moving off a neighbour is the placement layer's requirement, not scope churn.
The corpus run's last two findings came from industrial dynamics, which draws an alias of rri on short pipes. Deleting cpd turned pdd's end into a cloud where every position short of the valve covered the alias drawn on the pipe, while the space the stock took was free; the cloud end now searches both along the pipe and past its end, a pixel at a time, for the nearest clear spot. Restating cpd without psd re-attached psd, whose valve the author had drawn over another rri alias before the edit, and no clear valve position exists on that pipe. The audit charged it because the flow changed. Placement now counts, as consistency already did, only what the edit introduced: two shapes that already overlapped before it are the author's.
4fbf98b to
43f3716
Compare
|
Reviewed the substantive changes across Points I traced end to end without finding a defect:
Findings: none. Overall correctness: correct. No blocking issues. The PR reads as a coherent, test-driven repair sweep with each fix carrying its own red-then-green test and shrinking |
Review found three ways the incremental sync broke its own rules. A re-attached flow whose valve slid off a shape kept its old valve position in the layout cache, and when the same patch created an element, the settled-coordinate copy translated the whole pipe back to that spot, pulling its ends off the stock face and the cloud. The valve slide moved only the elements; the caches are now refreshed after it, as they already were for created flows, through one helper. A variable the view leaves out can still have a uid (MCP's open_project mints one for every variable), so the connector diff drew links into or out of it and the cloud diff made clouds of an undrawn flow at (0,0), each referencing no element. The connector diff now creates a link only between drawn variables, and the cloud diff creates a cloud only at a drawn pipe's end. The existing missing-connector test built its fixture without uids and could not reach this path, so the new test mints them the way production does. The branch that creates no element returned before re-bowing curved links, although a re-attached valve or a variable redrawn in place off a shape can move there too; it now turns them with their chord. The tests row both branches for every fix.
Edit scenarios composed equations by interpolating variable names bare, so a model with a name the lexer cannot read unquoted got a different edit from the one the scenario named: extending a stock named labor-force wrote labor-force * 0.1, a subtraction reading labor and force, and the new flow had no dependency on the stock. The runner does not compile, so the battery audited that edit silently. Every name a scenario writes into equation text now goes through ast::print_ident, the printer's own quoting rule, which becomes pub(crate) as the one spelling of a name in equation text. The test drives every scenario kind over a model whose names all need quotes and charges an equation error on any variable a step writes, with the production compiler as the oracle; deleting a parameter still leaves its readers with an unknown dependency, which is the edit, not a spelling.
Code reviewI reviewed the 6717-line diff focused on discrete bugs introduced by this PR. Areas examined most carefully:
The changes are large but heavily tested: 527 layout unit tests, the new FindingsNone flagged. Overall correctness verdictCorrect. |
edit_model's diagram sync swallowed a layout failure and returned, so the edit landed with the old diagram still in the file and nothing told the agent: added variables were missing from it and deleted ones still drawn, and an agent reading the diagram took it as current. A failed sync still never fails the edit, since the model data is correct, but it now adds a generic, model-kind warning prefixed "diagram sync:" naming the reason. The install step is split out so both of its arms are unit-tested; no MCP input is known to make the layout fail. Both edit_model and libsimlin's simlin_project_diagram_sync replaced a model's whole view list with the one synced view, silently dropping any other view a project carries (JSON and protobuf hold a list). They now replace only the first view, the one the layout syncs, and add one when the model has none.
The JupyterLab journey has failed on every CI run since the diagram editing overhaul landed (a8eb2a5 on its branch, 1da80fc on main): it opened the new aux's details and waited for the rendered equation preview. That commit canonicalizes the ident on each diagnostic before attaching it, so the aux's empty-equation error, raised under its display spelling "New Variable", now reaches the variable; a variable with an equation error opens its details on the raw equation editor, so there is no preview to click. The Editor behaves as designed and the journey had been passing only because the error never matched. The spec now expects the error and the editor directly.
1 participant
Why
Simlin draws and keeps a diagram for models agents and notebook users edit: after every MCP
edit_modelcall and every pysimlin patch,incremental_layoutbrings the existing view in line with the patched model. Diagrams generated from scratch are measured by the layout-quality metric and its harness, but a static score cannot say whether a sync did the right thing -- whether it left alone what the edit did not touch, kept the view consistent with the model, and put what it created somewhere sensible. The only evaluation of that path was a replay that adds chains, which never deletes, re-attaches, renames or changes a variable's kind.This branch builds that evaluation, uses it to find what the sync gets wrong, and fixes it.
The harness
layout::edit_auditstates the sync's contract from an edit's inputs and outputs alone, without readingincremental.rs:editing::invariants; only findings the edit introduced count, so an imported view's own inconsistencies are not charged to it;layout::edit_scenariosgenerates sixteen kinds of edit for any model -- restate a variable, add, delete or rename a parameter (with the rename operation and the way an agent without one does it), insert an intermediate, delete a flow or a middle stock, detach a flow, turn an aux into a stock, add a flow between stocks, close a loop, extend a chain, add a side flow, add a sector, add then undo -- and drives each throughapply_patchand the production sync rule, checking that two syncs of one edit agree and that an undo returns the original view.edit_scenarios_tests.rs) runs every scenario over thirteen fixtures -- hand-drawn views, and imported Vensim views (groupon, catastrophe, the beer game, bathtub statistics, alias1) with aliases, links the dependency extraction does not explain, variables the author did not draw, and flows meeting at one stock -- and pins every finding inKNOWN_DEFECTS, one row per fixture, scenario and finding naming its defect: a new finding fails, and so does a row that stops reproducing, so a fix deletes its rows. The audit's decision table (edit_audit_tests.rs) has a row per finding kind, each checked on a correct and a defective view built from a synced edit.layout_evalruns the same scenarios over the corpus and writesedits.html: each run's last step rendered before (removed elements marked) and after (created, changed, and every located finding marked).LAYOUT_EVAL_EDITS=0skips it.What it found, and the fixes
KNOWN_DEFECTSis empty. Each fix has its own commit with a test written red first, and deletes its rows.On the hand-drawn fixtures (54 findings at the start):
diff_connectorsanddiff_cloudsremoved every link and cloud and pushed the survivors back, clouds inHashSetorder, so restating a variable reordered the saved file and two syncs of one edit could differ. Surviving links and clouds now stay in place; created ones are appended in uid order.LayoutState::remove_for_rebuildkeeps the uid, position, links and aliases; the variable is redrawn at its old center, and a curved link whose endpoint moved keeps its bow.retarget_flowmoves only the ends that changed, through the editing core's ownheal,route_endandroute, so a tool edit's pipes are drawn by the same code as a touch edit's.total_population -> susceptible. Decided: a sync must not change parts of the diagram unrelated to the edit, so a missing connector is drawn only into a variable the patch names or where an end is drawn for the first time.On the imported views the corpus run exposed (94 findings once they joined the battery):
.x, stored as·x; a rename rewrote onlyself·x, so renamingareain hares and foxes left the module reading a name that no longer exists -- its input port silently took its default. This is an engine bug, not a diagram one: the patch's rename now treats the parent-scope spelling likeself.repeaters -> subscribersdisappeared on any edit, and undoing the edit could not bring it back. Such a link now stays unless the patch names its reader. Keeping them exposed that deleting a variable removed its aliases but not the links touching them; the deletion removes those itself now.And on the large imported models of the corpus, which the battery does not load (6 of 364 corpus runs after the fixes above):
b. It moves to the nearest clear spot when, and only when, its new shape there covers another shape; the audit charges a move of a rebuilt element only where the old center was clear.rrion the short pipe betweencpdandpdd's valve; deletingcpdleft no position short of the valve that cleared it, while the space the stock took was free. A cloud end now searches both along its pipe and past its end, a pixel at a time, for the nearest clear spot.cpdwithoutpsdre-attachespsd, whose valve the author had already drawn over anotherrrialias, and no clear valve position exists on that pipe. Placement now counts, as consistency already did, only what the edit introduced: two shapes that already overlapped before the edit are not charged.iad_net_flowdrew it (the edit names it), so the restate and add-then-undo scenarios claimed the view should return unchanged when it cannot. Scenarios now target drawn variables.MCP
EditModelgainsrenameVariable, mapping to the engine'sRenameVariable. Spelling a rename as remove + upsert drew the variable somewhere new with re-created links (population's layout cost 0.31 -> 0.54) and left the agent to rewrite every reader.UpdateStockFlowsdeliberately stays off:upsertStockalready sets a stock's flows, and upsert stays create-or-replace (an omittedinflows/outflowsmeans none).Evidence
cargo test -p simlin-engine --lib -- layout::(527 tests, including the battery and audit table), the engine integration layout tests (37), andpatch::testspass;cargo test -p simlin-mcp-corepasses with the newrename_variable_rewrites_readers_and_keeps_the_diagrame2e test. Every commit passed the pre-commit hook.an_edit_that_changes_no_structure_returns_the_view_byte_for_byte,syncing_one_edit_twice_produces_one_view,only_the_links_a_sync_creates_are_curved_for_a_loop,a_stock_added_to_a_drawn_chain_lands_clear_of_side_flows,a_variable_whose_kind_changes_is_redrawn_where_it_was,a_flow_rebuilt_for_a_new_attachment_keeps_its_links,a_detached_flow_end_becomes_a_cloud_clear_of_the_stock,deleting_a_middle_stock_leaves_the_flows_through_it_in_place,a_flow_created_between_two_stocks_routes_around_the_stocks_between,a_created_valve_lands_clear_of_a_parameter,rename_rewrites_a_parent_scope_module_source,an_untouched_alias_keeps_its_label_side,a_link_drawing_no_dependency_goes_only_with_an_edit_to_its_reader,deleting_a_variable_removes_the_links_of_its_aliases,clouds_of_flows_through_a_deleted_stock_land_clear_of_each_other,a_created_side_flow_cloud_lands_clear_of_a_stock,clouds_of_flows_leaving_a_deleted_stock_at_one_point_separate,a_reattached_flow_valve_lands_clear_of_a_parameter,a_variable_redrawn_as_a_larger_shape_moves_off_its_neighbour,scenarios_target_only_variables_the_view_draws,a_cloud_left_by_a_deleted_stock_steps_off_a_parameter_drawn_on_its_pipe, anda_sync_draws_a_missing_connector_only_where_the_edit_is_about_it(every arm of the connector and undrawn-variable rules).a_created_parameter_left_on_a_shape_moves_to_the_nearest_clear_spotpins the clearance pass's arms on hand-built elements; its production composition is the catastrophe battery row it deleted.test_incremental_combined_opsbuilt its patched model by hand, changing a flow's equation without its patch naming the flow -- an input production never supplies, which the unexplained-link rule then read as an author's link. It now derives the model throughapply_patchon a model carrying the synced view.LAYOUT_EVAL_SEEDS=1 LAYOUT_EVAL_REPLAY_STEPS=0, all 28 corpus models, 364 scenario runs): no finding on any model, and every planned step applied and synced. When the imported fixtures joined the battery, 161 of those runs had findings, all on imported multi-view views; after the battery's fixes, 6 (thyroid and industrial dynamics); after those, 2; now none.Fixes #340: the battery's module fixture (
hares_and_foxes) keeps both module elements through a fresh layout and every applicable scenario; the drop the issue describes no longer happens, and this is its regression coverage.What this does not establish
🤖 Generated with Claude Code
https://claude.ai/code/session_01LaUjFVSCaGJLiMetVwyjz5