From b9b475ab77caa84912d63802d4b1bb5d92232ccb Mon Sep 17 00:00:00 2001 From: AJ Isaacs Date: Mon, 5 Oct 2026 00:25:43 -0400 Subject: [PATCH] fix(cutting): keep plan installation behind the verified service Review of the atomic Apply found two commit-boundary gaps: - The plan installer checked only root program references, so a public caller could install a payload whose subprograms alias another live (even locked) part's program, or share settings between parts. The installer and its payload types are now internal; CuttingPlanService.Apply, which installs owned copies of independently replayed proposals, is the only public path. - A part placed on two plates in one scope was planned once per plate; one plate's install replaced the program the other plate verified as fixed. A part repeated anywhere in the scope is now invalid input. --- .../CNC/CuttingPlanning/CuttingPlanCommit.cs | 18 +++++--- .../CuttingPlanning/CuttingPlanCommitTests.cs | 43 +++++++++++++++++++ 2 files changed, 55 insertions(+), 6 deletions(-) diff --git a/OpenNest.Core/CNC/CuttingPlanning/CuttingPlanCommit.cs b/OpenNest.Core/CNC/CuttingPlanning/CuttingPlanCommit.cs index 3a99505..739e88d 100644 --- a/OpenNest.Core/CNC/CuttingPlanning/CuttingPlanCommit.cs +++ b/OpenNest.Core/CNC/CuttingPlanning/CuttingPlanCommit.cs @@ -8,7 +8,7 @@ using OpenNest.Geometry; namespace OpenNest.CNC.CuttingPlanning; /// An owned planned program for one unlocked part; ownership passes to the part on Apply. -public sealed class PlannedPartProgram +internal sealed class PlannedPartProgram { public PlannedPartProgram(Part part, Program program, CuttingParameters parameters) { @@ -23,7 +23,7 @@ public sealed class PlannedPartProgram } /// The verified order and planned programs for one plate, bound to its captured state. -public sealed class PlateCuttingPlan +internal sealed class PlateCuttingPlan { public PlateCuttingPlan(PlateCuttingState expected, IEnumerable order, IEnumerable programs = null) @@ -75,10 +75,12 @@ public sealed class CuttingCommitResult /// Installs verified cutting plans for a whole scope at once. Nothing is searched, emitted, /// rotated or regenerated here: inputs are validated and checked for freshness, bounds are /// staged, then order and programs are installed synchronously and published once per plate. +/// Internal: it checks root program references only, so payloads must be owned copies of +/// independently replayed proposals. CuttingPlanService.Apply is the public entry point. /// -public static class CuttingPlanCommit +internal static class CuttingPlanCommit { - public static CuttingCommitResult Apply(IEnumerable plans, CancellationToken token = default) => + internal static CuttingCommitResult Apply(IEnumerable plans, CancellationToken token = default) => Apply(plans, token, null); // beforeInstall is a test seam that runs inside the install boundary, before each program. @@ -113,6 +115,7 @@ public static class CuttingPlanCommit var staged = new List(scope.Length); var targets = new HashSet(ReferenceEqualityComparer.Instance); + var members = new HashSet(ReferenceEqualityComparer.Instance); var installed = new HashSet(ReferenceEqualityComparer.Instance); var live = new HashSet(scope.SelectMany(p => p.Expected.Order).Select(p => p.Program), ReferenceEqualityComparer.Instance); @@ -128,11 +131,14 @@ public static class CuttingPlanCommit { return Invalid(plate, "The planned order is not exactly the plate's current parts."); } - var members = new HashSet(order, ReferenceEqualityComparer.Instance); + // A part on two plates would let one plate's install change the other's verified program. + if (order.Any(part => !members.Add(part))) + return Invalid(plate, "A part appears on more than one plate in the commit scope."); + var onPlate = new HashSet(order, ReferenceEqualityComparer.Instance); var programs = new List<(Part, Program, Box, CuttingParameters)>(); foreach (var planned in plan.Programs) { - if (planned?.Part == null || !members.Contains(planned.Part) || !targets.Add(planned.Part)) + if (planned?.Part == null || !onPlate.Contains(planned.Part) || !targets.Add(planned.Part)) return Invalid(plate, "Planned programs must target distinct parts of their own plate."); if (planned.Part.LeadInsLocked) return Invalid(plate, "A locked part's program is retained exactly and cannot be replaced."); diff --git a/OpenNest.Tests/CuttingPlanning/CuttingPlanCommitTests.cs b/OpenNest.Tests/CuttingPlanning/CuttingPlanCommitTests.cs index 4325318..46e4f0d 100644 --- a/OpenNest.Tests/CuttingPlanning/CuttingPlanCommitTests.cs +++ b/OpenNest.Tests/CuttingPlanning/CuttingPlanCommitTests.cs @@ -179,6 +179,49 @@ public class CuttingPlanCommitTests Assert.False(ProgramContent.Equal(With("X"), With("x"))); } + [Fact] + public void Apply_PartSharedByTwoPlates_IsRefusedWithoutChange() + { + var parameters = ExplicitContourTests.Parameters(); + var clean = LeadPathValidationTests.Rectangle(0, 0, 2, 2); + var prepared = PreparedContours.Capture(clean, parameters); + var shared = new Part(new Drawing("same", clean), new Vector(5, 5)); + Assert.True(shared.RestoreLeadInProgram(prepared.Emit([prepared.ClosestEntry(0, new Vector(3, 1))]), false)); + var nest = new Nest(); + var regenerated = nest.CreatePlate(); + var fixedPlate = nest.CreatePlate(); + regenerated.Parts.Add(shared); + fixedPlate.Parts.Add(shared); + var results = new[] + { + CuttingPlanService.Plan(new CuttingPlanRequest(regenerated, confirmedParameters: parameters)), + CuttingPlanService.Plan(new CuttingPlanRequest(fixedPlate)) + }; + Assert.All(results, r => Assert.Equal(CuttingPlanStatus.Ready, r.Status)); + Assert.True(results[0].ProposedOrder[0].IsRegenerated); + var program = shared.Program; + var states = new[] { regenerated, fixedPlate }.Select(p => PlateCuttingState.Capture(p)).ToArray(); + + var commit = CuttingPlanService.Apply(results); + + // Installing through one plate would silently replace the other plate's fixed program. + Assert.Equal(CuttingCommitStatus.InvalidInput, commit.Status); + Assert.Same(program, shared.Program); + Assert.All(states, s => Assert.True(s.IsCurrent(), s.Difference())); + } + + [Fact] + public void CommitInstaller_IsOnlyReachableThroughTheVerifiedService() + { + // The installer validates root references only; owned, verified payloads come from + // CuttingPlanService.Apply. Public access would accept nested aliases of live programs. + Assert.False(typeof(CuttingPlanCommit).IsPublic); + Assert.False(typeof(PlateCuttingPlan).IsPublic); + Assert.False(typeof(PlannedPartProgram).IsPublic); + Assert.True(typeof(CuttingPlanService).GetMethod(nameof(CuttingPlanService.Apply), + [typeof(IEnumerable), typeof(CancellationToken)])!.IsPublic); + } + [Fact] public void Apply_LaterPlateStale_AppliesNothingAnywhere() {