From 23c330ef1b7a067fb1af86124ee569e302b74878 Mon Sep 17 00:00:00 2001 From: rldyourmnd Date: Thu, 10 Sep 2026 23:46:45 +0500 Subject: [PATCH] fix(delete): apply must rebuild the retirement evidence the approver signed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `gds repository delete --apply` could not succeed for any repository. The precondition observer is built twice — once when the plan is stored, once when it is applied — and the apply site set neither `retirement` nor `preserve`. Unread remote collections classify as unknown, which blocks; an empty preserved set turns every accepted loss back into a blocker. `Retirable` was false on every apply, and the failed observation surfaced as GDS_STALE_PLAN with an empty mismatch list. The accepted losses now travel on the transition, so they are inside the plan parameters and the plan digest the approval signs, and one constructor serves both paths so they cannot drift again. Claude-Session: https://claude.ai/code/session_01CKXKXND4zAgWisTtatyTHX --- .gds/bundle.lock.yaml | 10 ++--- .github/workflows/gds-ci.yml | 4 +- core/app/repository_delete.go | 49 ++++++++++++++------- core/app/repository_delete_observer_test.go | 39 ++++++++++++++++ core/cli/root.go | 5 ++- core/repository/lifecycle.go | 23 ++++++++++ core/repository/lifecycle_test.go | 43 ++++++++++++++++++ 7 files changed, 149 insertions(+), 24 deletions(-) create mode 100644 core/app/repository_delete_observer_test.go diff --git a/.gds/bundle.lock.yaml b/.gds/bundle.lock.yaml index e62254d..b38287f 100644 --- a/.gds/bundle.lock.yaml +++ b/.gds/bundle.lock.yaml @@ -5,14 +5,14 @@ bundle: version: "0.9.0-dev" release_sequence: 0 channel: "development" - source_tree_digest: "sha256:6108aa7953daaa83e6f0497ce49ad328faa73f3b5711db3ab808ca982997f1f7" - digest: "sha256:76519a731f68615c821b7b79d2d4aa4dc98735a1fc8349e7b8e1fff27337bd34" + source_tree_digest: "sha256:e88904c3b4b3654f40b30843ea28f1ab33bb4ee45db6d6b0f0f89076857ba54f" + digest: "sha256:1e6b426bb6796d36de00d07037fbfc2591f2f6af30d63f2bd65ae091dbf82973" projection: - input_digest: "sha256:3ac5dff25cf43ff0c55f7ab2f13538402b8401f09da4a1b816f5ff898c17df03" - output_digest: "sha256:b975910bc9ac02a54efa6316051bf5f9c2909ff0d6ddf60a4e759f5b3faf98b7" + input_digest: "sha256:c939fde7254664217bc73f2b990ce7e73db38359ae62bcc8ca690ab6ca066b92" + output_digest: "sha256:0f5697325f6623916cb575aa8e1b2e0a3422635de596a1fd048d2776deda131c" files: - path: ".gds/compiled-policy.json" digest: "sha256:7ba2962e8afdf99eaf32582cd866f394abff2e24728b0644f013ec67d478b521" - path: ".github/workflows/gds-ci.yml" - digest: "sha256:01d2bb480a1d52e044ded2a2ee8a07b136e612ca030b81da4eab73398e57ebfd" + digest: "sha256:a6a19aa0ac91644efe450663924f229a7f474d016e58764be2b561608b4bd8b4" diff --git a/.github/workflows/gds-ci.yml b/.github/workflows/gds-ci.yml index a55164a..c150c79 100644 --- a/.github/workflows/gds-ci.yml +++ b/.github/workflows/gds-ci.yml @@ -1,8 +1,8 @@ # GENERATED FILE - DO NOT EDIT DIRECTLY # generator: gds # bundle: 0.9.0-dev -# source-tree-digest: sha256:6108aa7953daaa83e6f0497ce49ad328faa73f3b5711db3ab808ca982997f1f7 -# input-digest: sha256:3ac5dff25cf43ff0c55f7ab2f13538402b8401f09da4a1b816f5ff898c17df03 +# source-tree-digest: sha256:e88904c3b4b3654f40b30843ea28f1ab33bb4ee45db6d6b0f0f89076857ba54f +# input-digest: sha256:c939fde7254664217bc73f2b990ce7e73db38359ae62bcc8ca690ab6ca066b92 # output-digest: sha256:8c045e745cc69b731bc695a4a9d58a48c10f1ab7dd85b7354db7bfd0e072711c # edit-source: # - .gds/repository.yaml diff --git a/core/app/repository_delete.go b/core/app/repository_delete.go index f47a7f3..7a1e365 100644 --- a/core/app/repository_delete.go +++ b/core/app/repository_delete.go @@ -89,6 +89,31 @@ type repositoryDeleteObserver struct { transition repositoryworkflow.ProviderTransition } +// newRepositoryDeleteObserver is the single construction site for the delete +// precondition observer. Planning and apply both go through it because they +// drifted apart once already: the apply path was built without the retirement +// reader and without the preserved set, so every remote collection read as +// unknown and every accepted loss read as blocking. `Retirable` was then false +// forever, which the engine surfaced as GDS_STALE_PLAN with an empty mismatch +// list — a failed observation wearing the name of a state change. Both inputs +// now come from the transition, which the plan stores and the approval signs. +func (services *Services) newRepositoryDeleteObserver( + root string, + transition repositoryworkflow.ProviderTransition, + options RepositoryDeleteOptions, + reader repositoryworkflow.ProviderReader, +) repositoryDeleteObserver { + return repositoryDeleteObserver{ + services: services, root: root, inventoryRoot: transition.AnalysisRoot, + maxDepth: options.MaxDepth, maxRepositories: options.MaxRepositories, + concurrency: options.Concurrency, + reader: reader, + retirement: retirementReaderFor(reader), + preserve: transition.PreservedIdentities, + transition: transition, + } +} + func (observer repositoryDeleteObserver) Observe( ctx context.Context, repositoryID string, @@ -229,15 +254,10 @@ func (services *Services) PlanRepositoryDelete( return domain.InternalError(command, err) } engine := operations.NewDefaultEngine( - store, services.Schemas, repositoryDeleteObserver{ - services: services, root: current.root, inventoryRoot: current.transition.AnalysisRoot, - maxDepth: options.MaxDepth, maxRepositories: options.MaxRepositories, - concurrency: options.Concurrency, - reader: providerRuntime.readers[current.transition.CurrentInstallation], - retirement: retirementReaderFor(providerRuntime.readers[current.transition.CurrentInstallation]), - preserve: options.PreserveIdentities, - transition: current.transition, - }, nil, options.DeviceID, options.SessionID) + store, services.Schemas, services.newRepositoryDeleteObserver( + current.root, current.transition, options, + providerRuntime.readers[current.transition.CurrentInstallation], + ), nil, options.DeviceID, options.SessionID) engine.Now = services.Now if err := engine.PutPlan(ctx, plan); err != nil { return operationFailureEnvelope(command, err) @@ -336,12 +356,10 @@ func (services *Services) applyRepositoryDelete( return githubMutationRuntimeError(command, err) } handler := &repositoryworkflow.ProviderHandler{Readers: providerRuntime.readers, Writer: writer} - observer := repositoryDeleteObserver{ - services: services, root: current.root, inventoryRoot: transition.AnalysisRoot, - maxDepth: options.MaxDepth, maxRepositories: options.MaxRepositories, - concurrency: options.Concurrency, - reader: providerRuntime.readers[transition.CurrentInstallation], transition: transition, - } + observer := services.newRepositoryDeleteObserver( + current.root, transition, options, + providerRuntime.readers[transition.CurrentInstallation], + ) engine := operations.NewDefaultEngine( store, services.Schemas, observer, map[string]operations.ActionHandler{repositoryworkflow.ProviderLifecycleAction: handler}, @@ -446,6 +464,7 @@ func (services *Services) repositoryDeleteContext( return repositoryDeleteContext{}, []domain.Finding{*finding} } transition.AnalysisRoot = physicalInventoryRoot + transition.PreservedIdentities = append([]string(nil), options.PreserveIdentities...) index, indexFindings := services.completeRelationshipIndex(ctx, DiscoveryOptions{ Root: physicalInventoryRoot, MaxDepth: options.MaxDepth, MaxRepositories: options.MaxRepositories, Concurrency: options.Concurrency, diff --git a/core/app/repository_delete_observer_test.go b/core/app/repository_delete_observer_test.go new file mode 100644 index 0000000..ba944fd --- /dev/null +++ b/core/app/repository_delete_observer_test.go @@ -0,0 +1,39 @@ +package app + +import ( + "testing" + + repositoryworkflow "github.com/NDDev-OpenNetwork/github-device-sync/core/repository" +) + +// The delete precondition observer is built twice — once when the plan is +// stored and once when it is applied — and the two sites drifted apart, which +// made every approved deletion fail as GDS_STALE_PLAN. Planning and apply must +// derive the same observer from the same stored transition. +func TestDeleteObserverCarriesTheApprovedRetirementDeclaration(t *testing.T) { + services := &Services{} + transition := repositoryworkflow.ProviderTransition{ + Operation: repositoryworkflow.DeleteOperation, RepositoryID: "repo_01ABCDEFGHJKMNPQRSTVWXYZ01", + ProviderRepositoryID: 42, CurrentInstallation: "installation:github-personal", + CurrentOwner: "example-owner", CurrentName: "example", CurrentLifecycle: "archived", + TargetInstallation: "installation:github-personal", + TargetOwner: "example-owner", TargetName: "example", TargetLifecycle: "tombstoned", + AnalysisRoot: "/verified-estate", + PreservedIdentities: []string{"commits:unpushed", "ref:refs/tags/v0.6.10"}, + } + options := RepositoryDeleteOptions{MaxDepth: 8, MaxRepositories: 2000, Concurrency: 4} + + observer := services.newRepositoryDeleteObserver("/verified-estate/example", transition, options, nil) + + if len(observer.preserve) != len(transition.PreservedIdentities) { + t.Fatalf("preserve=%#v transition=%#v", observer.preserve, transition.PreservedIdentities) + } + for index, identity := range transition.PreservedIdentities { + if observer.preserve[index] != identity { + t.Fatalf("preserve[%d]=%q expected %q", index, observer.preserve[index], identity) + } + } + if observer.inventoryRoot != transition.AnalysisRoot { + t.Fatalf("inventoryRoot=%q expected %q", observer.inventoryRoot, transition.AnalysisRoot) + } +} diff --git a/core/cli/root.go b/core/cli/root.go index 284873b..cc508e9 100644 --- a/core/cli/root.go +++ b/core/cli/root.go @@ -2269,10 +2269,11 @@ func (executor *executor) repositoryDeleteCommand() *cobra.Command { }) } if !plan && (child.Flags().Changed("inventory-root") || - child.Flags().Changed("confirm-repository-id") || child.Flags().Changed("confirm-provider-id")) { + child.Flags().Changed("confirm-repository-id") || + child.Flags().Changed("confirm-provider-id") || child.Flags().Changed("preserve")) { return domain.NewEnvelope("gds repository delete", domain.ExitInput, nil, domain.Finding{ Code: "GDS_REPOSITORY_DELETE_INPUT_CONFLICT", Severity: domain.SeverityHigh, - Message: "Inventory and confirmation values are planning inputs and cannot alter a stored plan.", + Message: "Inventory, confirmation and preservation values are planning inputs and cannot alter a stored plan.", }) } switch { diff --git a/core/repository/lifecycle.go b/core/repository/lifecycle.go index 1eb2a89..1ba48db 100644 --- a/core/repository/lifecycle.go +++ b/core/repository/lifecycle.go @@ -36,6 +36,13 @@ type ProviderTransition struct { TargetName string `json:"target_name"` TargetLifecycle string `json:"target_lifecycle"` AnalysisRoot string `json:"analysis_root,omitempty"` + // PreservedIdentities is the exact set of retirement identities the + // operator accepted losing when the plan was built. It lives on the + // transition, and therefore inside the plan parameters and the plan + // digest, because the apply path has to rebuild the same retirement + // evidence the approver saw. Recomputing it from apply-time flags would + // let an approved plan be applied against a different declaration. + PreservedIdentities []string `json:"preserved_identities,omitempty"` } func ValidateDelete(current domain.RepositoryAnchor) (ProviderTransition, []domain.Finding) { @@ -76,6 +83,13 @@ func Parameters(transition ProviderTransition) map[string]any { if transition.AnalysisRoot != "" { parameters["analysis_root"] = transition.AnalysisRoot } + if len(transition.PreservedIdentities) != 0 { + identities := make([]any, 0, len(transition.PreservedIdentities)) + for _, identity := range transition.PreservedIdentities { + identities = append(identities, identity) + } + parameters["preserved_identities"] = identities + } return map[string]any{"repository_provider": parameters} } @@ -102,6 +116,15 @@ func StepTransition(step operations.Step) (ProviderTransition, error) { result.TargetName, _ = raw["target_name"].(string) result.TargetLifecycle, _ = raw["target_lifecycle"].(string) result.AnalysisRoot, _ = raw["analysis_root"].(string) + if identities, ok := raw["preserved_identities"].([]any); ok { + for _, identity := range identities { + value, valid := identity.(string) + if !valid || value == "" { + return ProviderTransition{}, errors.New("repository provider parameters are invalid") + } + result.PreservedIdentities = append(result.PreservedIdentities, value) + } + } if (result.Operation != RenameOperation && result.Operation != TransferOperation && result.Operation != ArchiveOperation && result.Operation != DeleteOperation) || result.ProviderRepositoryID < 1 || result.CurrentInstallation == "" || result.CurrentOwner == "" || diff --git a/core/repository/lifecycle_test.go b/core/repository/lifecycle_test.go index 39e7d16..2a97bbe 100644 --- a/core/repository/lifecycle_test.go +++ b/core/repository/lifecycle_test.go @@ -115,3 +115,46 @@ func assertTransitionFinding(t *testing.T, findings []domain.Finding, code strin } t.Fatalf("missing %s in %#v", code, findings) } + +func TestDeletePlanCarriesTheAcceptedLossesThroughItsParameters(t *testing.T) { + current := lifecycleAnchor() + current.Repository.Lifecycle = "archived" + transition, findings := ValidateDelete(current) + if len(findings) != 0 { + t.Fatalf("findings=%#v", findings) + } + // What the operator accepted losing is part of what the approver signs, so + // it has to survive into the stored plan: the apply path rebuilds the + // retirement evidence from it, and an empty set makes every accepted loss + // read as blocking again. + transition.AnalysisRoot = "/verified-estate" + transition.PreservedIdentities = []string{"commits:unpushed", "ref:refs/tags/v0.6.10"} + decoded, err := StepTransition(operations.Step{ + RepositoryID: transition.RepositoryID, Action: ProviderLifecycleAction, + Parameters: Parameters(transition), + }) + if err != nil || !reflect.DeepEqual(decoded, transition) { + t.Fatalf("decoded=%#v transition=%#v err=%v", decoded, transition, err) + } +} + +func TestDeleteParametersRejectANonStringAcceptedLoss(t *testing.T) { + current := lifecycleAnchor() + current.Repository.Lifecycle = "archived" + transition, findings := ValidateDelete(current) + if len(findings) != 0 { + t.Fatalf("findings=%#v", findings) + } + parameters := Parameters(transition) + provider, ok := parameters["repository_provider"].(map[string]any) + if !ok { + t.Fatalf("parameters=%#v", parameters) + } + provider["preserved_identities"] = []any{"commits:unpushed", 7} + if _, err := StepTransition(operations.Step{ + RepositoryID: transition.RepositoryID, Action: ProviderLifecycleAction, + Parameters: parameters, + }); err == nil { + t.Fatal("a non-string accepted loss must not decode into a preservation declaration") + } +}