Complete issue #23 push safety and acceptance evidence

CHANGELOG.md

@@ -7,6 +7,15 @@ and this project follows [Semantic Versioning](https://semver.org/spec/v2.0.0.ht
 
 ## [Unreleased]
 
+### Fixed
+
+- Publish to the canonical upstream `template/memory-bank/` payload root,
+  retaining legacy-root fallback for compatible upstream repositories.
+- Reject all unresolved downstream Git conflict statuses before an upstream
+  publication plan can mutate state.
+- Reject symlinked `memory-bank/` and `.repo` checkout ancestry, and provide
+  corrective next steps in push preflight diagnostics.
+
 ## [1.4.0] - 2026-07-24
 
 ### Added

internal/cli/cli_test.go

@@ -73,6 +73,19 @@ func TestRootHelpAndVersion(t *testing.T) {
 		if !strings.Contains(stdout.String(), test.want) {
 			t.Fatalf("unexpected stdout for %v: %q", test.arguments, stdout.String())
 		}
+		if test.arguments[0] == "--help" && !strings.Contains(stdout.String(), "push    Publish managed Memory Bank changes upstream through a PR") {
+			t.Fatalf("root help does not document push: %q", stdout.String())
+		}
+	}
+}
+
+func TestPushHelpDocumentsDryRun(t *testing.T) {
+	var stdout, stderr bytes.Buffer
+	if exitCode := Run([]string{"push", "--help"}, "test", &stdout, &stderr); exitCode != 0 {
+		t.Fatalf("help exit=%d stderr=%q", exitCode, stderr.String())
+	}
+	if !strings.Contains(stderr.String(), "Usage: memory-bank-cli push") || !strings.Contains(stderr.String(), "without mutating checkout, remotes, or GitHub") {
+		t.Fatalf("push help is incomplete: %q", stderr.String())
 	}
 }
 

internal/push/push.go

@@ -56,7 +56,7 @@ func Run(options Options) (Report, error) {
 		now = time.Now
 	}
 	if _, err := run(options.RepoRoot, "git", "rev-parse", "--show-toplevel"); err != nil {
-		return Report{}, fmt.Errorf("push must run inside a Git repository: %w", err)
+		return Report{}, fmt.Errorf("push must run inside a Git repository; run it from a repository or pass --repo-root: %w", err)
 	}
 	checkout, err := safeCheckout(options.RepoRoot)
 	if err != nil {
@@ -67,26 +67,21 @@ func Run(options Options) (Report, error) {
 	}
 	remote, err := run(checkout, "git", "remote", "get-url", "origin")
 	if err != nil || strings.TrimSpace(remote) == "" {
-		return Report{}, fmt.Errorf("upstream checkout has no valid origin remote")
+		return Report{}, errors.New("upstream checkout has no valid origin remote; configure memory-bank/.repo origin to the target GitHub repository")
 	}
 	githubRepo, err := githubRepository(strings.TrimSpace(remote))
 	if err != nil {
 		return Report{}, err
 	}
 	if !options.DryRun {
 		if identity, err := run(checkout, "gh", "repo", "view", githubRepo, "--json", "id"); err != nil || strings.TrimSpace(identity) == "" {
-			return Report{}, errors.New("upstream origin is not an accessible GitHub repository for PR creation")
+			return Report{}, fmt.Errorf("upstream origin %q is not accessible for PR creation; run gh auth login and verify repository access", githubRepo)
 		}
 	}
 	defaultBranch, err := defaultBranch(run, checkout)
 	if err != nil {
 		return Report{}, err
 	}
-	if !options.DryRun {
-		if _, err := run(checkout, "git", "fetch", "origin", defaultBranch+":refs/remotes/origin/"+defaultBranch); err != nil {
-			return Report{}, fmt.Errorf("refresh upstream default branch: %w", err)
-		}
-	}
 	payloadRoot, err := selectPayloadRootAt(run, checkout, "origin/"+defaultBranch)
 	if err != nil {
 		return Report{}, err
@@ -127,7 +122,17 @@ func Run(options Options) (Report, error) {
 		}
 	}
 	if len(included) == 0 {
-		return report, errors.New("no managed Memory Bank changes to publish")
+		return report, errors.New("no managed Memory Bank changes to publish; edit a managed path or inspect exclusions with --dry-run")
+	}
+	if _, err := run(checkout, "git", "fetch", "origin", defaultBranch+":refs/remotes/origin/"+defaultBranch); err != nil {
+		return report, fmt.Errorf("refresh upstream default branch: %w", err)
+	}
+	refreshedPayloadRoot, err := selectPayloadRootAt(run, checkout, "origin/"+defaultBranch)
+	if err != nil {
+		return report, err
+	}
+	if refreshedPayloadRoot != payloadRoot {
+		return report, fmt.Errorf("upstream payload root changed from %q to %q while refreshing the default branch; retry the command", payloadRoot, refreshedPayloadRoot)
 	}
 	makeBranch := options.BranchName
 	if makeBranch == nil {
@@ -228,15 +233,24 @@ func Run(options Options) (Report, error) {
 }
 
 func safeCheckout(root string) (string, error) {
-	path := filepath.Join(root, "memory-bank", ".repo")
-	info, err := os.Lstat(path)
-	if err != nil {
-		return "", fmt.Errorf("inspect upstream checkout: %w", err)
-	}
-	if info.Mode()&os.ModeSymlink != 0 || !info.IsDir() {
-		return "", errors.New("memory-bank/.repo must be a real directory, not a symlink")
+	memoryBank := filepath.Join(root, "memory-bank")
+	checkout := filepath.Join(memoryBank, ".repo")
+	for _, candidate := range []struct {
+		path  string
+		label string
+	}{
+		{path: memoryBank, label: "memory-bank"},
+		{path: checkout, label: "memory-bank/.repo"},
+	} {
+		info, err := os.Lstat(candidate.path)
+		if err != nil {
+			return "", fmt.Errorf("inspect %s: %w; clone the upstream repository into memory-bank/.repo", candidate.label, err)
+		}
+		if info.Mode()&os.ModeSymlink != 0 || !info.IsDir() {
+			return "", fmt.Errorf("%s must be a real directory, not a symlink; replace it with a local upstream checkout", candidate.label)
+		}
 	}
-	return path, nil
+	return checkout, nil
 }
 
 func githubRepository(remote string) (string, error) {
@@ -246,11 +260,11 @@ func githubRepository(remote string) (string, error) {
 	} else if strings.HasPrefix(value, "https://github.com/") {
 		value = strings.TrimPrefix(value, "https://github.com/")
 	} else {
-		return "", fmt.Errorf("upstream origin must be a GitHub repository: %q", remote)
+		return "", fmt.Errorf("upstream origin must be a GitHub repository: %q; set memory-bank/.repo origin to a GitHub SSH or HTTPS URL", remote)
 	}
 	parts := strings.Split(value, "/")
 	if len(parts) != 2 || parts[0] == "" || parts[1] == "" {
-		return "", fmt.Errorf("invalid GitHub origin: %q", remote)
+		return "", fmt.Errorf("invalid GitHub origin: %q; use an owner/repository GitHub URL", remote)
 	}
 	return value, nil
 }
@@ -274,39 +288,52 @@ func clean(run func(string, string, ...string) (string, error), dir string) erro
 	if filepath.Clean(absDir) != filepath.Clean(absTop) {
 		return fmt.Errorf("memory-bank/.repo must be its own Git worktree (got %q, want %q)", absTop, absDir)
 	}
-	status, err := run(dir, "git", "status", "--porcelain")
+	conflicts, err := run(dir, "git", "diff", "--name-only", "--diff-filter=U")
 	if err != nil {
 		return err
 	}
-	if strings.TrimSpace(status) != "" {
-		return errors.New("upstream checkout is dirty; commit, stash, or discard its changes first")
+	if strings.TrimSpace(conflicts) != "" {
+		return errors.New("upstream checkout has unresolved conflicts; resolve them before running push")
 	}
-	conflicts, err := run(dir, "git", "diff", "--name-only", "--diff-filter=U")
+	status, err := run(dir, "git", "status", "--porcelain")
 	if err != nil {
 		return err
 	}
-	if strings.TrimSpace(conflicts) != "" {
-		return errors.New("upstream checkout has unresolved conflicts")
+	if strings.TrimSpace(status) != "" {
+		return errors.New("upstream checkout is dirty; commit, stash, or discard its changes first")
 	}
 	return nil
 }
 
 func defaultBranch(run func(string, string, ...string) (string, error), checkout string) (string, error) {
 	ref, err := run(checkout, "git", "symbolic-ref", "--short", "refs/remotes/origin/HEAD")
 	if err != nil {
-		return "", fmt.Errorf("resolve upstream default branch: %w", err)
+		return "", fmt.Errorf("resolve upstream default branch: %w; run git -C memory-bank/.repo remote set-head origin --auto", err)
 	}
 	branch := strings.TrimPrefix(strings.TrimSpace(ref), "origin/")
 	if branch == "" || branch == ref {
-		return "", errors.New("upstream origin/HEAD does not name a default branch")
+		return "", errors.New("upstream origin/HEAD does not name a default branch; run git -C memory-bank/.repo remote set-head origin --auto")
 	}
 	if _, err := run(checkout, "git", "rev-parse", "--verify", "origin/"+branch); err != nil {
-		return "", fmt.Errorf("default branch %q is not available locally: %w", branch, err)
+		return "", fmt.Errorf("default branch %q is not available locally: %w; fetch origin and retry", branch, err)
 	}
 	return branch, nil
 }
 
 func selectPayloadRoot(checkout string) (string, error) {
+	canonical := filepath.Join(checkout, "template", "memory-bank")
+	if info, err := os.Lstat(canonical); err == nil {
+		if !info.IsDir() || info.Mode()&os.ModeSymlink != 0 {
+			return "", errors.New("upstream payload root \"template/memory-bank\" must be a real directory")
+		}
+		template, err := os.Lstat(filepath.Join(checkout, "template"))
+		if err != nil || !template.IsDir() || template.Mode()&os.ModeSymlink != 0 {
+			return "", errors.New("upstream payload root \"template/memory-bank\" has an unsafe parent directory")
+		}
+		return "template/memory-bank", nil
+	} else if !errors.Is(err, os.ErrNotExist) {
+		return "", err
+	}
 	var roots []string
 	for _, candidate := range []string{"memory-bank-template", "memory-bank"} {
 		info, err := os.Lstat(filepath.Join(checkout, candidate))
@@ -322,12 +349,20 @@ func selectPayloadRoot(checkout string) (string, error) {
 		roots = append(roots, candidate)
 	}
 	if len(roots) != 1 {
-		return "", fmt.Errorf("upstream checkout must contain exactly one payload root (memory-bank-template or memory-bank), found %v", roots)
+		return "", fmt.Errorf("upstream checkout must contain template/memory-bank or exactly one legacy payload root (memory-bank-template or memory-bank), found %v; check out the upstream default branch and fix its payload layout", roots)
 	}
 	return roots[0], nil
 }
 
 func selectPayloadRootAt(run func(string, string, ...string) (string, error), checkout, ref string) (string, error) {
+	canonical := "template/memory-bank"
+	out, err := run(checkout, "git", "ls-tree", "-d", "--name-only", ref, "--", canonical)
+	if err != nil {
+		return "", fmt.Errorf("inspect upstream payload root %q: %w", canonical, err)
+	}
+	if strings.TrimSpace(out) == canonical {
+		return canonical, nil
+	}
 	var roots []string
 	for _, candidate := range []string{"memory-bank-template", "memory-bank"} {
 		out, err := run(checkout, "git", "ls-tree", "-d", "--name-only", ref, "--", candidate)
@@ -339,7 +374,7 @@ func selectPayloadRootAt(run func(string, string, ...string) (string, error), ch
 		}
 	}
 	if len(roots) != 1 {
-		return "", fmt.Errorf("default branch must contain exactly one payload root (memory-bank-template or memory-bank), found %v", roots)
+		return "", fmt.Errorf("default branch must contain template/memory-bank or exactly one legacy payload root (memory-bank-template or memory-bank), found %v; verify the upstream repository payload layout", roots)
 	}
 	return roots[0], nil
 }
@@ -357,8 +392,8 @@ func changedPaths(run func(string, string, ...string) (string, error), root stri
 			continue
 		}
 		status, paths := line[:2], line[3:]
-		if strings.Contains(status, "U") {
-			return nil, fmt.Errorf("downstream path has unresolved Git conflict: %q", paths)
+		if unmergedStatus(status) {
+			return nil, fmt.Errorf("downstream path has unresolved Git conflict: %q; resolve the conflict before running push", paths)
 		}
 		if strings.Contains(status, "R") {
 			if index+1 >= len(records) || records[index+1] == "" {
@@ -390,6 +425,15 @@ func changedPaths(run func(string, string, ...string) (string, error), root stri
 	return paths, nil
 }
 
+func unmergedStatus(status string) bool {
+	switch status {
+	case "DD", "AU", "UD", "UA", "DU", "AA", "UU":
+		return true
+	default:
+		return false
+	}
+}
+
 func branchName(now time.Time) (string, error) {
 	bytes := make([]byte, 6)
 	if _, err := rand.Read(bytes); err != nil {