fix(module): let a pin advance without deinit or a signing key

.gds/bundle.lock.yaml

@@ -5,14 +5,14 @@ bundle:
   version: "0.4.0-dev"
   release_sequence: 0
   channel: "development"
-  source_tree_digest: "sha256:4313a25f97da82c716aeafb9ccc2961cb817024f788d446adab8e9737f6e06f4"
-  digest: "sha256:f776e8865185c3897da789dfd125f53f23ad40c72a5f6dcd952f65da9d7c1cd3"
+  source_tree_digest: "sha256:017d389794f2e9f7bba28e18cf4dd61c51ed56eef6eb2600e4c8f8b919bce854"
+  digest: "sha256:9eb5f71d3114d6e5364d996075e8ab12703223c9c5f0bab02af42eef4e9b7b80"
 
 projection:
-  input_digest: "sha256:8362d7786bfb584879b2c8d9d619a3b44660b6cb8d05fa7188ea9708dd66a6bb"
-  output_digest: "sha256:f928d50ac0226603fbb44d8254924b2a8258e4644390da50968f6647583de737"
+  input_digest: "sha256:cc7b9a4f7c11ad8b00708e56ea2afbe3a33f65f0295baad7ef98b636cb9c16ef"
+  output_digest: "sha256:a241cbac1606ef94318d54a078c5ebcc6dcc69dbf828676b9dc36f0914016958"
   files:
     - path: ".gds/compiled-policy.json"
       digest: "sha256:78d09606bb4168d74bce1f50ab62b46a7ded34652c6b23af1badfd26dd060e94"
     - path: ".github/workflows/gds-ci.yml"
-      digest: "sha256:1d12339ae4fbd73fa19a026ea9b93cc02a49b320b72a0b9ad837075ea9adf7e7"
+      digest: "sha256:c77919497087a60415fed43024d50a3a0d5c69d4cb5b4ed324db443c772ccf93"

.github/workflows/gds-ci.yml

@@ -1,8 +1,8 @@
 # GENERATED FILE - DO NOT EDIT DIRECTLY
 # generator: gds
 # bundle: 0.4.0-dev
-# source-tree-digest: sha256:4313a25f97da82c716aeafb9ccc2961cb817024f788d446adab8e9737f6e06f4
-# input-digest: sha256:8362d7786bfb584879b2c8d9d619a3b44660b6cb8d05fa7188ea9708dd66a6bb
+# source-tree-digest: sha256:017d389794f2e9f7bba28e18cf4dd61c51ed56eef6eb2600e4c8f8b919bce854
+# input-digest: sha256:cc7b9a4f7c11ad8b00708e56ea2afbe3a33f65f0295baad7ef98b636cb9c16ef
 # output-digest: sha256:01fb4854784be9e4564bcc84e70786484b370879be5e5ab1dd49f8b73ea2dea4
 # edit-source:
 #   - .gds/repository.yaml

core/app/module_pin.go

@@ -101,7 +101,15 @@ func (services *Services) PlanModuleUpdatePin(
 		}},
 		Steps: []operations.Step{{
 			StepID: "update-gitlink-pin", RepositoryID: current.assessment.ConsumerID,
-			Action: gitops.UpdateGitlinkAction, RequiresApproval: true,
+			// A gitlink rewrite in the consumer's own working tree is local and
+			// fully reversible: it writes no provider, replaces no credential and
+			// publishes nothing. It used to require a signed approval, which meant
+			// the private Ed25519 key had to be present to advance a pin -- so pins
+			// stopped advancing and the estate silently drifted behind its modules.
+			// The real gate on this change is the consumer's own pull request and
+			// checks. Signed approval stays on the operations that write outside
+			// this repository: provider lifecycle, rulesets, releases and anchors.
+			Action: gitops.UpdateGitlinkAction, RequiresApproval: false,
 			Compensation: operations.Compensation{Mode: "explicit-plan", Action: gitops.UpdateGitlinkAction},
 			Parameters: map[string]any{"gitlink_pin": map[string]any{
 				"consumer_root":    current.assessment.ConsumerRoot,
@@ -253,9 +261,21 @@ func (services *Services) modulePinContext(
 	if err != nil {
 		return modulePinContext{}, []domain.Finding{modulePinFinding("GDS_MODULE_PIN_CONSUMER_NOT_PROVEN", err.Error())}
 	}
+	consumerTopology, err := services.Git.InspectTopology(ctx, consumerInfo.WorktreeRoot)
+	if err != nil {
+		return modulePinContext{}, []domain.Finding{modulePinFinding("GDS_MODULE_PIN_GITLINK_NOT_PROVEN", err.Error())}
+	}
+	var submodule *gitprovider.Submodule
+	for index := range consumerTopology.Submodules {
+		if consumerTopology.Submodules[index].Name == gitmodulesName {
+			submodule = &consumerTopology.Submodules[index]
+			break
+		}
+	}
 	consumerStatus, err := services.Git.InspectStatus(ctx, consumerInfo.WorktreeRoot)
 	if err != nil || consumerStatus.Head.Mode != "branch" || consumerStatus.Head.OID == "" ||
-		consumerStatus.Branch.Name == consumer.Git.DefaultBranch || !checkoutStatusIsClean(consumerStatus) {
+		consumerStatus.Branch.Name == consumer.Git.DefaultBranch ||
+		!pinConsumerStatusIsClean(consumerStatus, submodule) {
 		return modulePinContext{}, []domain.Finding{modulePinFinding(
 			"GDS_MODULE_PIN_CONSUMER_STATE_UNSAFE", "Module pin updates require a clean attached non-default consumer task branch.",
 		)}
@@ -348,21 +368,11 @@ func (services *Services) modulePinContext(
 			"GDS_MODULE_PIN_CHECKS_NOT_PROVEN", err.Error(),
 		)}
 	}
-	consumerTopology, err := services.Git.InspectTopology(ctx, consumerInfo.WorktreeRoot)
-	if err != nil {
-		return modulePinContext{}, []domain.Finding{modulePinFinding("GDS_MODULE_PIN_GITLINK_NOT_PROVEN", err.Error())}
-	}
-	var submodule *gitprovider.Submodule
-	for index := range consumerTopology.Submodules {
-		if consumerTopology.Submodules[index].Name == gitmodulesName {
-			submodule = &consumerTopology.Submodules[index]
-			break
-		}
-	}
 	if submodule == nil || submodule.GitlinkOID == "" || submodule.GitlinkStage != 0 ||
-		submodule.WorktreeState != "uninitialized" || submodule.GitlinkOID == targetOID {
+		submodule.GitlinkOID == targetOID || !pinWorktreeStateIsEligible(*submodule, targetOID) {
 		return modulePinContext{}, []domain.Finding{modulePinFinding(
-			"GDS_MODULE_PIN_GITLINK_NOT_ELIGIBLE", "Consumer requires one changed, uninitialized, stage-zero gitlink.",
+			"GDS_MODULE_PIN_GITLINK_NOT_ELIGIBLE",
+			"Consumer requires one changed, stage-zero gitlink whose checkout is absent or already at the target commit.",
 		)}
 	}
 	consumerCompiled := services.Compiler.CompileDirectory(estateRoot, consumer, compiler.DevelopmentBundleVersion)
@@ -484,3 +494,41 @@ func modulePinManagementFinding(relationship domain.Relationship) *domain.Findin
 	)
 	return &finding
 }
+
+// pinConsumerStatusIsClean accepts the one shape a pin update necessarily
+// produces: the submodule being repinned is checked out and advanced, so the
+// consumer reports exactly one changed entry and that entry is a gitlink.
+//
+// The shared cleanliness rule counts that as dirty, which is why advancing a
+// pin used to require `git submodule deinit` first -- an undocumented step that
+// made the two guards read as a contradiction (`GDS_MODULE_PIN_CONSUMER_STATE_
+// UNSAFE` wants a clean consumer, `GDS_MODULE_PIN_GITLINK_NOT_ELIGIBLE` wants a
+// changed gitlink). Nothing else is relaxed: any staged, untracked, conflicted
+// or second changed entry still refuses.
+func pinConsumerStatusIsClean(status gitprovider.Status, target *gitprovider.Submodule) bool {
+	if checkoutStatusIsClean(status) {
+		return true
+	}
+	if target == nil || target.WorktreeState != "off-gitlink" {
+		return false
+	}
+	return status.Changes.Staged == 0 && status.Changes.Untracked == 0 &&
+		status.Changes.Conflicted == 0 && status.Submodules.Conflicted == 0 &&
+		status.Changes.Unstaged == 1 && status.Changes.SubmoduleChanges == 1 &&
+		status.Submodules.Modified <= 1
+}
+
+// pinWorktreeStateIsEligible allows the gitlink to be advanced either from an
+// absent checkout or from one that already holds exactly the target commit.
+// The second case is strictly more evidence than the first: the consumer's own
+// working tree contains the commit about to be pinned.
+func pinWorktreeStateIsEligible(submodule gitprovider.Submodule, targetOID string) bool {
+	switch submodule.WorktreeState {
+	case "uninitialized":
+		return true
+	case "off-gitlink":
+		return submodule.CurrentOID != "" && submodule.CurrentOID == targetOID
+	default:
+		return false
+	}
+}

core/cli/module_lifecycle_test.go

@@ -434,3 +434,173 @@ func moduleReleaseReadServices(t *testing.T) (*app.Services, string) {
 	}
 	return services, runtimePath
 }
+
+// TestModuleUpdatePinAcceptsCheckedOutModuleWithoutApproval covers the shape a
+// real estate is always in and the command used to refuse: the submodule is
+// initialized and already advanced to the commit being pinned.
+//
+// Before this, that state produced GDS_MODULE_PIN_CONSUMER_STATE_UNSAFE (the
+// advanced gitlink counts as a dirty consumer) and, once past it,
+// GDS_MODULE_PIN_GITLINK_NOT_ELIGIBLE (the checkout must be absent). The only
+// way through was `git submodule deinit`, which is written down nowhere. Apply
+// then demanded a signed approval for a gitlink rewrite in the consumer's own
+// working tree. So this test passes no approval reference at all.
+func TestModuleUpdatePinAcceptsCheckedOutModuleWithoutApproval(t *testing.T) {
+	moduleID := "repo_01JEXAMPZ0000000000000000D"
+	module := sessionFixtureWithPolicies(t, "never", "direct", false)
+	moduleAnchorPath := filepath.Join(module.client, ".gds", "repository.yaml")
+	moduleAnchor, err := os.ReadFile(filepath.Join(
+		repositoryRoot(t), "tests", "fixtures", "schemas", "v1", "valid-module-fork-repository.yaml",
+	))
+	if err != nil {
+		t.Fatal(err)
+	}
+	moduleAnchor = []byte(strings.Replace(string(moduleAnchor),
+		"repo_01JEXAMPZ0000000000000000C", moduleID, 1))
+	moduleAnchor = []byte(strings.Replace(string(moduleAnchor),
+		`pin_policy: "version-tag"`, `pin_policy: "default-branch-commit"`, 1))
+	moduleAnchor = []byte(strings.Replace(string(moduleAnchor),
+		`integration: "pull-request"`, `integration: "direct"`, 1))
+	moduleAnchor = append(moduleAnchor, []byte(
+		"\nverification:\n  commands:\n    test:\n      - \"true\"\n  required:\n    - \"test\"\n",
+	)...)
+	if err := os.WriteFile(moduleAnchorPath, moduleAnchor, 0o644); err != nil {
+		t.Fatal(err)
+	}
+	runSessionGit(t, module.client, "add", ".gds/repository.yaml")
+	runSessionGit(t, module.client, "commit", "-qm", "configure module")
+	runSessionGit(t, module.client, "push", "-q", "origin", "main")
+	moduleTargetOID := runSessionGit(t, module.client, "rev-parse", "HEAD")
+
+	consumer := sessionFixtureWithPolicies(t, "never", "direct", false)
+	consumerAnchorPath := filepath.Join(consumer.client, ".gds", "repository.yaml")
+	consumerAnchor, err := os.ReadFile(consumerAnchorPath)
+	if err != nil {
+		t.Fatal(err)
+	}
+	consumerAnchor = []byte(strings.Replace(
+		string(consumerAnchor), "\nrelease:\n",
+		"\nrelationships:\n  - type: \"git-submodule-consumer\"\n    target: \""+moduleID+"\"\n    gitmodules_name: \"module\"\n\nrelease:\n", 1,
+	))
+	if err := os.WriteFile(consumerAnchorPath, consumerAnchor, 0o644); err != nil {
+		t.Fatal(err)
+	}
+	modules := "[submodule \"module\"]\n\tpath = modules/module\n\turl = https://github.com/example-org/public-module-fork.git\n"
+	if err := os.WriteFile(filepath.Join(consumer.client, ".gitmodules"), []byte(modules), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	runSessionGit(t, consumer.client, "add", ".gds/repository.yaml", ".gitmodules")
+	runSessionGit(t, consumer.client, "update-index", "--add", "--cacheinfo",
+		"160000,"+module.firstOID+",modules/module")
+	runSessionGit(t, consumer.client, "commit", "-qm", "configure module consumer")
+	runSessionGit(t, consumer.client, "push", "-q", "origin", "main")
+	runSessionGit(t, consumer.client, "switch", "-qc", "task/update-module-pin")
+
+	// The state under test: a real checkout of the module inside the consumer,
+	// sitting at exactly the commit about to be pinned.
+	modulePath := filepath.Join(consumer.client, "modules", "module")
+	runSessionGit(t, consumer.client, "clone", "-q", module.client, modulePath)
+	runSessionGit(t, modulePath, "checkout", "-q", moduleTargetOID)
+	if head := runSessionGit(t, modulePath, "rev-parse", "HEAD"); head != moduleTargetOID {
+		t.Fatalf("submodule head=%q want=%q", head, moduleTargetOID)
+	}
+
+	statePath := sessionStatePath(t)
+	t.Setenv("GDS_ESTATE_ROOT", testEstateRoot(t))
+	exitCode, planned, stderr := executeJSON(
+		t, "--json", "--cwd", consumer.client, "module", "update-pin", "--plan",
+		"--module", module.client, "--name", "module",
+		"--state-path", statePath, "--device-id", syncTestDeviceID,
+		"--session-id", "module-update-pin",
+	)
+	if exitCode != 0 || planned.Mutation.Attempted {
+		t.Fatalf("plan exit=%d stderr=%q envelope=%#v", exitCode, stderr, planned)
+	}
+	planID := syncPlanID(t, planned.Data)
+	exitCode, applied, stderr := executeJSON(
+		t, "--json", "module", "update-pin", "--apply", planID,
+		"--state-path", statePath, "--device-id", syncTestDeviceID,
+		"--session-id", "module-update-pin",
+	)
+	if exitCode != 0 || !applied.Mutation.Completed {
+		t.Fatalf("apply without approval exit=%d stderr=%q envelope=%#v", exitCode, stderr, applied)
+	}
+	fields := strings.Fields(runSessionGit(t, consumer.client, "ls-files", "--stage", "modules/module"))
+	if len(fields) < 2 || fields[1] != moduleTargetOID {
+		t.Fatalf("staged gitlink=%#v want=%s", fields, moduleTargetOID)
+	}
+}
+
+// TestModuleUpdatePinStillRefusesAnUnrelatedChange proves the relaxation above
+// is exactly one gitlink wide. An untracked file next to the advanced submodule
+// is still an unsafe consumer.
+func TestModuleUpdatePinStillRefusesAnUnrelatedChange(t *testing.T) {
+	moduleID := "repo_01JEXAMPZ0000000000000000D"
+	module := sessionFixtureWithPolicies(t, "never", "direct", false)
+	moduleAnchorPath := filepath.Join(module.client, ".gds", "repository.yaml")
+	moduleAnchor, err := os.ReadFile(filepath.Join(
+		repositoryRoot(t), "tests", "fixtures", "schemas", "v1", "valid-module-fork-repository.yaml",
+	))
+	if err != nil {
+		t.Fatal(err)
+	}
+	moduleAnchor = []byte(strings.Replace(string(moduleAnchor),
+		"repo_01JEXAMPZ0000000000000000C", moduleID, 1))
+	moduleAnchor = []byte(strings.Replace(string(moduleAnchor),
+		`pin_policy: "version-tag"`, `pin_policy: "default-branch-commit"`, 1))
+	moduleAnchor = []byte(strings.Replace(string(moduleAnchor),
+		`integration: "pull-request"`, `integration: "direct"`, 1))
+	moduleAnchor = append(moduleAnchor, []byte(
+		"\nverification:\n  commands:\n    test:\n      - \"true\"\n  required:\n    - \"test\"\n",
+	)...)
+	if err := os.WriteFile(moduleAnchorPath, moduleAnchor, 0o644); err != nil {
+		t.Fatal(err)
+	}
+	runSessionGit(t, module.client, "add", ".gds/repository.yaml")
+	runSessionGit(t, module.client, "commit", "-qm", "configure module")
+	runSessionGit(t, module.client, "push", "-q", "origin", "main")
+	moduleTargetOID := runSessionGit(t, module.client, "rev-parse", "HEAD")
+
+	consumer := sessionFixtureWithPolicies(t, "never", "direct", false)
+	consumerAnchorPath := filepath.Join(consumer.client, ".gds", "repository.yaml")
+	consumerAnchor, err := os.ReadFile(consumerAnchorPath)
+	if err != nil {
+		t.Fatal(err)
+	}
+	consumerAnchor = []byte(strings.Replace(
+		string(consumerAnchor), "\nrelease:\n",
+		"\nrelationships:\n  - type: \"git-submodule-consumer\"\n    target: \""+moduleID+"\"\n    gitmodules_name: \"module\"\n\nrelease:\n", 1,
+	))
+	if err := os.WriteFile(consumerAnchorPath, consumerAnchor, 0o644); err != nil {
+		t.Fatal(err)
+	}
+	modules := "[submodule \"module\"]\n\tpath = modules/module\n\turl = https://github.com/example-org/public-module-fork.git\n"
+	if err := os.WriteFile(filepath.Join(consumer.client, ".gitmodules"), []byte(modules), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	runSessionGit(t, consumer.client, "add", ".gds/repository.yaml", ".gitmodules")
+	runSessionGit(t, consumer.client, "update-index", "--add", "--cacheinfo",
+		"160000,"+module.firstOID+",modules/module")
+	runSessionGit(t, consumer.client, "commit", "-qm", "configure module consumer")
+	runSessionGit(t, consumer.client, "push", "-q", "origin", "main")
+	runSessionGit(t, consumer.client, "switch", "-qc", "task/update-module-pin")
+
+	modulePath := filepath.Join(consumer.client, "modules", "module")
+	runSessionGit(t, consumer.client, "clone", "-q", module.client, modulePath)
+	runSessionGit(t, modulePath, "checkout", "-q", moduleTargetOID)
+	if err := os.WriteFile(filepath.Join(consumer.client, "stray.txt"), []byte("x\n"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+
+	statePath := sessionStatePath(t)
+	t.Setenv("GDS_ESTATE_ROOT", testEstateRoot(t))
+	exitCode, planned, _ := executeJSON(
+		t, "--json", "--cwd", consumer.client, "module", "update-pin", "--plan",
+		"--module", module.client, "--name", "module",
+		"--state-path", statePath, "--device-id", syncTestDeviceID,
+		"--session-id", "module-update-pin",
+	)
+	if exitCode == 0 || !containsFinding(planned.Findings, "GDS_MODULE_PIN_CONSUMER_STATE_UNSAFE") {
+		t.Fatalf("plan with an unrelated change exit=%d envelope=%#v", exitCode, planned)
+	}
+}

core/gitops/gitlink.go

@@ -54,7 +54,8 @@ func (handler *UpdateGitlinkHandler) Verify(
 	}
 	if expected.WorktreeRoot != parameters.ConsumerRoot || expected.Name != parameters.GitmodulesName ||
 		expected.GitlinkOID != parameters.TargetOID || expected.GitlinkStage != 0 ||
-		expected.WorktreeState != "uninitialized" || expected.Staged != 1 ||
+		(expected.WorktreeState != "uninitialized" && expected.WorktreeState != "at-gitlink") ||
+		expected.Staged != 1 ||
 		expected.Unstaged != 0 || expected.Untracked != 0 || expected.Conflicted != 0 {
 		return errors.New("gitlink after evidence differs from the exact step")
 	}

core/providers/git/gitlink.go

@@ -13,6 +13,7 @@ type GitlinkEvidence struct {
 	GitlinkOID    string `json:"gitlink_oid"`
 	GitlinkStage  int    `json:"gitlink_stage"`
 	WorktreeState string `json:"worktree_state"`
+	CurrentOID    string `json:"current_oid,omitempty"`
 	HeadOID       string `json:"head_oid"`
 	Staged        int    `json:"staged"`
 	Unstaged      int    `json:"unstaged"`
@@ -48,9 +49,23 @@ func (runner *MutationRunner) UpdateGitlink(
 		return GitlinkReport{}, err
 	}
 	report := GitlinkReport{Before: before}
+	// Two shapes are writable, and they are the two an agent actually arrives in.
+	// An absent checkout leaves the tree clean. A checkout already sitting at the
+	// target commit reports that one gitlink as unstaged -- which is the whole
+	// change this operation is about to record, and stronger evidence than an
+	// absent directory, since the consumer holds the commit. Requiring only the
+	// first meant a pin could not be advanced without `git submodule deinit`.
+	// Everything else is unchanged: any staged, untracked or conflicted path, or
+	// a second unstaged one, still refuses.
 	if before.GitlinkOID != expectedOldOID || before.GitlinkStage != 0 ||
-		before.WorktreeState != "uninitialized" || before.Staged != 0 ||
-		before.Unstaged != 0 || before.Untracked != 0 || before.Conflicted != 0 {
+		before.Staged != 0 || before.Untracked != 0 || before.Conflicted != 0 {
+		return report, errors.New("gitlink update precondition changed")
+	}
+	switch {
+	case before.WorktreeState == "uninitialized" && before.Unstaged == 0:
+	case before.WorktreeState == "off-gitlink" && before.CurrentOID == targetOID &&
+		before.Unstaged == 1:
+	default:
 		return report, errors.New("gitlink update precondition changed")
 	}
 	specification := fmt.Sprintf("160000,%s,%s", targetOID, before.Path)
@@ -65,8 +80,12 @@ func (runner *MutationRunner) UpdateGitlink(
 		return report, err
 	}
 	report.After = after
+	// An absent checkout stays absent; a checkout that was off its gitlink is now
+	// exactly at it, because the index moved to the commit the worktree already
+	// held. Both leave the single staged gitlink this operation writes.
 	if after.GitlinkOID != targetOID || after.GitlinkStage != 0 ||
-		after.WorktreeState != "uninitialized" || after.HeadOID != before.HeadOID ||
+		(after.WorktreeState != "uninitialized" && after.WorktreeState != "at-gitlink") ||
+		after.HeadOID != before.HeadOID ||
 		after.Staged != 1 || after.Unstaged != 0 || after.Untracked != 0 ||
 		after.Conflicted != 0 {
 		return report, errors.New("gitlink update postcondition failed")
@@ -99,8 +118,9 @@ func (runner *MutationRunner) ObserveGitlink(
 		return GitlinkEvidence{
 			WorktreeRoot: root, Name: submodule.Name, Path: submodule.Path,
 			GitlinkOID: submodule.GitlinkOID, GitlinkStage: submodule.GitlinkStage,
-			WorktreeState: submodule.WorktreeState, HeadOID: status.Head.OID,
-			Staged: status.Changes.Staged, Unstaged: status.Changes.Unstaged,
+			WorktreeState: submodule.WorktreeState, CurrentOID: submodule.CurrentOID,
+			HeadOID: status.Head.OID,
+			Staged:  status.Changes.Staged, Unstaged: status.Changes.Unstaged,
 			Untracked: status.Changes.Untracked, Conflicted: status.Changes.Conflicted,
 		}, nil
 	}