engine: delete the diagnostics round trip (row B of the round-trips plan)

.claude/skills/address-feedback/SKILL.md

@@ -101,7 +101,7 @@ Ignore suggestions that:
 - Would add unnecessary complexity
 - The reviewer convinced itself weren't actually problems
 
-**Deferred feedback**: Only defer feedback that is genuinely unrelated to this PR's changes -- pre-existing issues in untouched code, future feature requests, or theoretical concerns about code paths this PR does not introduce or modify. For each deferred item, spawn the `track-issue` agent (via the Task tool with `subagent_type: "track-issue"`) with a detailed description.
+**Deferred feedback**: The default is to FIX what a review surfaces, including things the review found by accident -- see "Discovered Issues" in the root `CLAUDE.md`. Defer only feedback that is genuinely unrelated to this PR's changes AND too large to fold into it: pre-existing issues in untouched code, future feature requests, or theoretical concerns about code paths this PR does not introduce or modify. Deferring is an explicit call, not a default: say what you are deferring and why. For each deferred item, spawn the `track-issue` agent (via the Task tool with `subagent_type: "track-issue"`) with a detailed description.
 
 **CRITICAL**: P0/P1/P2 feedback about code introduced or modified by THIS PR must NEVER be deferred. If a reviewer flags a correctness, data-loss, or behavioral bug in code that this branch touches, fix it in this review cycle. Deferring P1 feedback on your own changes is not acceptable -- it means shipping a known bug. When in doubt about whether feedback is "in scope", err on the side of fixing it.
 

CLAUDE.md

@@ -121,11 +121,18 @@ IMPORTANT: If feedback seems non-actionable, it means you need comments explaini
 ### libsimlin API Design
 Keep the FFI surface small and orthogonal. Prefer composable primitives over bulk endpoints. Do NOT add bulk/batch variants to paper over caller-side concurrency issues.
 
-## Tracking Discovered Issues
+## Discovered Issues
 
-When you discover something wrong or concerning during your work -- tech debt, design limitations, broken tooling, missing CI checks, unintended consequences of a committed design, deferred review feedback -- it must be explicitly tracked. Never silently drop these observations.
+When you discover something wrong or concerning during your work -- tech debt, a latent bug, design limitations, broken tooling, missing CI checks, unintended consequences of a committed design, deferred review feedback -- **fix it as part of the work**. Never silently drop these observations, and never file an issue as a substitute for fixing one.
 
-Spawn the `track-issue` agent (via the Task tool with `subagent_type: "track-issue"`) with a description of the problem. The agent checks for duplicates in GitHub issues and [docs/tech-debt.md](/docs/tech-debt.md), then files the item if it's not already tracked. Using a sub-agent preserves your context on the main task.
+Filing defers the cost without reducing it, and the context needed to fix a problem is at its cheapest the moment you find it. Name what you fixed in the commit message or PR body (`Fixes #<n>` when an issue already exists).
+
+Two things are NOT covered by "fix it", and both are explicit conversations rather than silent decisions:
+
+- **The fix is too large to fold in** -- it would swamp the branch's review surface, or it belongs to a different subsystem. Say so, with a cost estimate, and sequence it: its own commit, its own PR, or (if that is what the user wants) tracked for later. Do not make that call quietly.
+- **The fix is not yours to make** -- it needs a product decision, or access you do not have. Surface it.
+
+When something genuinely does need tracking, spawn the `track-issue` agent (via the Task tool with `subagent_type: "track-issue"`) with a description of the problem. It checks for duplicates in GitHub issues and [docs/tech-debt.md](/docs/tech-debt.md) and files the item, keeping your context on the main task.
 
 ## Generated/Noise Paths
 

docs/design-plans/2026-05-13-macros.md

@@ -60,6 +60,7 @@ Vensim macros become a first-class, persistent concept in the Simlin engine -- i
 - **macros.AC5.4 Success:** A macro shadowing a builtin (`SSHAPE`, `RAMP FROM TO`) resolves to the macro; the builtin is not invoked.
 - **macros.AC5.5 Success:** A macro defined after its first use (`macro_trailing_definition`) still resolves and simulates.
 - **macros.AC5.6 Failure:** A call to a name that is neither a macro, a stdlib function, nor a builtin reports an "unknown function or macro" error.
+- **macros.AC5.7 Failure:** A macro-marked model whose body instantiates a module (`Variable::Module`) is rejected at registry-build time, naming the macro and the offending module variable. A cycle-safety rule rather than a taste rule: `db::project_module_graph` -- the gate every compile / diagnostic / analysis entry point consults so a module cycle surfaces as `CircularDependency` instead of salsa's dependency-graph cycle panic -- records only EXPLICIT module edges, because it must stay parse-free. A macro CALL is an implicit edge, so `mac ->(explicit module)-> u ->(macro call)-> mac` was a cycle the gate reported as absent and every entry point then aborted on (fatal under `panic = "abort"`). Not redundant with AC5.2: that cycle runs through a NON-macro model, which the macro-to-macro call graph cannot express, and the macro set is otherwise valid. The rejection is deliberately broader than the cycle -- an acyclic module inside a macro is rejected too -- and that cost is real rather than zero: the shape is reachable from an ordinary XMILE file (the `<macro>` content model is shared with `<model>`, so a `<module>` written inside a `<macro>` is passed through unfiltered rather than synthesized), Simlin's own XMILE writer round-trips it, and an acyclic instance compiles and simulates correctly. It is rejected anyway for two reasons: narrowing to only-when-cyclic requires a second reachability analysis inside `MacroRegistry::build` that must agree with `db::project_module_graph`'s, and the back edge it would have to see is the macro CALL -- discoverable only by parsing every model's equations, i.e. the same dependency-list cost that ruled out widening the graph; and a macro is a *template*, so instantiating a sub-model inside one is dubious on its own terms, making this a language rule with a cycle-safety motivation rather than a workaround. How many real models this affects is a judgement, not a measurement -- Stella emits no `<macro>` and xmutil emits them only from Vensim `:MACRO:` blocks, which cannot contain modules, so the population is hand-written XMILE or a third-party writer: small, but not empty. The MDL importer cannot produce the shape, though not because "Vensim macros have no modules" -- its multi-output materializer does mint `Variable::Module`; it is simply never run over a macro body. A module *targeting* a macro model (how a multi-output invocation works) is unaffected; only a module *inside* a macro-marked model is rejected.
 
 ### macros.AC6: Validation corpus and consumer floor
 - **macros.AC6.1 Success:** All six `test/test-models/tests/macro_*` fixtures are wired into the active test suite and pass.

src/libsimlin/src/lib.rs

@@ -308,6 +308,9 @@ impl From<engine::ErrorCode> for SimlinErrorCode {
             // wire Generic code; the specific engine code and the message
             // carry the detail (GH #905).
             engine::ErrorCode::UnknownElementSubscript => SimlinErrorCode::Generic,
+            // A macro body instantiating a module likewise collapses to the wire
+            // Generic code; the engine code and the message carry the detail.
+            engine::ErrorCode::MacroContainsModule => SimlinErrorCode::Generic,
         }
     }
 }

src/libsimlin/src/patch.rs

@@ -404,6 +404,15 @@ pub(crate) fn gather_error_details_with_db(
     all_errors
 }
 
+/// The code reported for a rejected patch: the first Error-severity diagnostic,
+/// else the VM validation error.
+///
+/// "First" is now deterministic. `collect_all_diagnostics` emits the
+/// project-level failures (bad unit declarations, an invalid macro set) ahead of
+/// the per-model passes, where they used to surface in whatever order the salsa
+/// accumulator DFS happened to walk. Accept/reject is unchanged -- the set of
+/// Error-severity diagnostics is the same -- but a project carrying both a
+/// project-level and a per-model error now reports the project-level code.
 fn first_error_code(
     diagnostics: &[engine::db::Diagnostic],
     sim_error: Option<&engine::Error>,