From 83e1bcf0effe4f58fe7a52cf676f229aad8d9544 Mon Sep 17 00:00:00 2001 From: Nikhil Sonti Date: Thu, 2 Jul 2026 14:31:28 -0700 Subject: [PATCH 1/4] feat(patches): bpatch apply auto-annotates; harden annotate against conflicts and junk apply now creates feature commits automatically after a conflict-free run (--no-annotate opts out; filtered applies never auto-annotate). The intent survives a conflict pause via resolve state, so continue/skip finish the annotation apply asked for. sync is unchanged. annotate itself gains three guards that caused 'apply && annotate' to misbehave: - refuses to run while a conflict resolution is pending instead of committing a half-applied tree (reject files were ending up in feature commits) - filters untracked junk (*.rej, *.orig, .llm/, .browseros-patchignore) through the same ignore set extract uses - reports changed files no feature claims as 'unclaimed' instead of silently leaving the checkout dirty --- .../browseros/tools/patch/cmd/annotate.go | 7 + packages/browseros/tools/patch/cmd/apply.go | 14 +- packages/browseros/tools/patch/cmd/common.go | 6 +- .../browseros/tools/patch/cmd/continue.go | 4 + packages/browseros/tools/patch/cmd/skip.go | 4 + .../tools/patch/internal/engine/annotate.go | 58 +++- .../tools/patch/internal/engine/apply.go | 96 +++++-- .../patch/internal/engine/engine_test.go | 248 ++++++++++++++++++ .../tools/patch/internal/resolve/resolve.go | 3 + 9 files changed, 406 insertions(+), 34 deletions(-) diff --git a/packages/browseros/tools/patch/cmd/annotate.go b/packages/browseros/tools/patch/cmd/annotate.go index 84d6bf1aa3..8c57627c3d 100644 --- a/packages/browseros/tools/patch/cmd/annotate.go +++ b/packages/browseros/tools/patch/cmd/annotate.go @@ -69,6 +69,13 @@ func printAnnotateResult(ws workspace.Entry, result *engine.AnnotateResult) { fmt.Printf(" %-24s %s\n", skipped.Name, skipped.Reason) } } + if len(result.Unclaimed) > 0 { + fmt.Println(ui.Warning("Unclaimed changes (no feature owns these; left uncommitted):")) + for _, rel := range result.Unclaimed { + fmt.Printf(" %s\n", rel) + } + fmt.Println(ui.Hint(`Claim them in bos_build/features.yaml, then re-run; "browseros-patch feature lint" checks coverage.`)) + } if result.CommitsCreated == 0 { fmt.Println(ui.Hint("No commits created.")) } diff --git a/packages/browseros/tools/patch/cmd/apply.go b/packages/browseros/tools/patch/cmd/apply.go index 76edddae5f..82e69ef182 100644 --- a/packages/browseros/tools/patch/cmd/apply.go +++ b/packages/browseros/tools/patch/cmd/apply.go @@ -13,11 +13,13 @@ func init() { var reset bool var changed string var rangeEnd string + var noAnnotate bool command := &cobra.Command{ Use: "apply [checkout] [-- files...]", Annotations: map[string]string{"group": "Core:"}, - Short: "Apply repo patches to a checkout", + Short: "Apply repo patches to a checkout and create feature commits", Example: ` browseros-patch apply ch1 + browseros-patch apply ch1 --no-annotate browseros-patch apply ch1 -- chrome/browser/browser.cc browseros-patch apply --src /path/to/chromium/src`, Args: cobra.ArbitraryArgs, @@ -41,7 +43,10 @@ func init() { ChangedRef: changed, RangeEnd: rangeEnd, Filters: filters, - Progress: commandProgress(cmd), + // A filtered apply is surgical; a full annotate would sweep + // changes outside the requested files into feature commits. + AutoAnnotate: !noAnnotate && len(filters) == 0, + Progress: commandProgress(cmd), }) if err != nil { return err @@ -58,6 +63,10 @@ func init() { } fmt.Println(ui.Hint(`Run "browseros-patch continue" after fixing the current conflict.`)) } + if result.Annotate != nil { + fmt.Println() + printAnnotateResult(ws, result.Annotate) + } }); err != nil { return err } @@ -68,5 +77,6 @@ func init() { command.Flags().BoolVar(&reset, "reset", false, "Reset patched files to BASE_COMMIT before applying") command.Flags().StringVar(&changed, "changed", "", "Apply only patches changed in the given repo commit") command.Flags().StringVar(&rangeEnd, "range-end", "", "End revision when using --changed as a range start") + command.Flags().BoolVar(&noAnnotate, "no-annotate", false, "Skip feature-commit annotation after apply (also skipped when -- files filters are given)") rootCmd.AddCommand(command) } diff --git a/packages/browseros/tools/patch/cmd/common.go b/packages/browseros/tools/patch/cmd/common.go index 66ff2e01fa..0b54f0e8de 100644 --- a/packages/browseros/tools/patch/cmd/common.go +++ b/packages/browseros/tools/patch/cmd/common.go @@ -82,7 +82,11 @@ Capture flow (turn checkout changes into patches): 3. browseros-patch publish -m "chore: sync patches" # commit + push chromium_patches Feature commit flow (turn checkout changes into feature commits): -1. browseros-patch annotate ch1 # commit changed files for all bos_build/features.yaml features +1. browseros-patch apply ch1 annotates automatically after a conflict-free apply; pass --no-annotate to skip. + A filtered apply (-- files...) never auto-annotates. continue/skip finish the annotation a paused apply asked for. +2. browseros-patch annotate ch1 # standalone: commit changed files for all bos_build/features.yaml features +3. annotate refuses to run while a conflict resolution is pending; finish continue/skip/abort first. +4. Changed files no feature claims are reported as "unclaimed" and left uncommitted; claim them in bos_build/features.yaml. Pool refresh flow (rebuild browseros branch before leasing a checkout): 1. browseros-patch status ch1 --json # check patches_freshness diff --git a/packages/browseros/tools/patch/cmd/continue.go b/packages/browseros/tools/patch/cmd/continue.go index 20da3591ef..36e860666c 100644 --- a/packages/browseros/tools/patch/cmd/continue.go +++ b/packages/browseros/tools/patch/cmd/continue.go @@ -35,6 +35,10 @@ func init() { fmt.Printf(" %s\n", conflict.ChromiumPath) } } + if result.Annotate != nil { + fmt.Println() + printAnnotateResult(ws, result.Annotate) + } printStashOutcome(result) }); err != nil { return err diff --git a/packages/browseros/tools/patch/cmd/skip.go b/packages/browseros/tools/patch/cmd/skip.go index eb7485133d..741c616e71 100644 --- a/packages/browseros/tools/patch/cmd/skip.go +++ b/packages/browseros/tools/patch/cmd/skip.go @@ -35,6 +35,10 @@ func init() { fmt.Printf(" %s\n", conflict.ChromiumPath) } } + if result.Annotate != nil { + fmt.Println() + printAnnotateResult(ws, result.Annotate) + } printStashOutcome(result) }); err != nil { return err diff --git a/packages/browseros/tools/patch/internal/engine/annotate.go b/packages/browseros/tools/patch/internal/engine/annotate.go index 3e2841b3cb..761b54202f 100644 --- a/packages/browseros/tools/patch/internal/engine/annotate.go +++ b/packages/browseros/tools/patch/internal/engine/annotate.go @@ -8,6 +8,7 @@ import ( "github.com/browseros-ai/BrowserOS/packages/browseros/tools/patch/internal/git" "github.com/browseros-ai/BrowserOS/packages/browseros/tools/patch/internal/patch" "github.com/browseros-ai/BrowserOS/packages/browseros/tools/patch/internal/repo" + "github.com/browseros-ai/BrowserOS/packages/browseros/tools/patch/internal/resolve" "github.com/browseros-ai/BrowserOS/packages/browseros/tools/patch/internal/workspace" "gopkg.in/yaml.v3" ) @@ -26,6 +27,9 @@ type AnnotateResult struct { FeaturesSkipped int `json:"features_skipped"` Committed []AnnotateCommittedFeature `json:"committed"` Skipped []AnnotateSkippedFeature `json:"skipped"` + // Unclaimed lists changed files no feature owns; they stay uncommitted + // so the checkout remains dirty until features.yaml claims them. + Unclaimed []string `json:"unclaimed,omitempty"` } type AnnotateCommittedFeature struct { @@ -48,11 +52,22 @@ type annotateFileSet struct { } // Annotate creates Chromium checkout commits grouped by the repo feature registry. +// It refuses to run mid-conflict-resolution: committing a half-applied tree +// would bake reject files and partial patches into feature history. func Annotate(ctx context.Context, opts AnnotateOptions) (*AnnotateResult, error) { + if resolve.Exists(opts.Workspace.Path) { + return nil, fmt.Errorf( + "conflict resolution is in progress for %s; finish it with \"browseros-patch continue\", \"browseros-patch skip\", or \"browseros-patch abort\" before annotating", + opts.Workspace.Name) + } features, featuresFile, err := LoadFeatures(opts.Repo) if err != nil { return nil, err } + ignore, err := patch.LoadIgnoreSet(opts.Repo.Root, nil) + if err != nil { + return nil, err + } result := &AnnotateResult{ Workspace: opts.Workspace.Name, FeaturesFile: featuresFile, @@ -71,7 +86,7 @@ func Annotate(ctx context.Context, opts AnnotateOptions) (*AnnotateResult, error result.FeaturesSkipped++ continue } - changes, err := annotateChanges(ctx, opts.Workspace.Path) + changes, err := annotateChanges(ctx, opts.Workspace.Path, ignore) if err != nil { return nil, err } @@ -106,9 +121,32 @@ func Annotate(ctx context.Context, opts AnnotateOptions) (*AnnotateResult, error }) result.CommitsCreated++ } + unclaimed, err := unclaimedChanges(ctx, opts.Workspace.Path, ignore) + if err != nil { + return nil, err + } + result.Unclaimed = unclaimed return result, nil } +// unclaimedChanges lists what is still dirty after every feature committed its +// files — changes no feature in the registry claims. +func unclaimedChanges(ctx context.Context, workspacePath string, ignore *patch.IgnoreSet) ([]string, error) { + changes, err := annotateChanges(ctx, workspacePath, ignore) + if err != nil { + return nil, err + } + seen := map[string]bool{} + var paths []string + for _, change := range changes { + for _, rel := range changeReportPaths(change) { + appendUniquePath(&paths, seen, rel) + } + } + slices.Sort(paths) + return paths, nil +} + func mappingValue(node *yaml.Node, key string) *yaml.Node { if node == nil { return nil @@ -153,8 +191,22 @@ func stringSequence(node *yaml.Node, key string) []string { return items } -func annotateChanges(ctx context.Context, workspacePath string) ([]git.FileChange, error) { - return git.StatusPorcelain(ctx, workspacePath, nil) +// annotateChanges returns working-tree changes eligible for feature commits. +// Untracked junk (reject files, logs, .browseros-patchignore patterns) is +// filtered like extract does; tracked modifications always pass through. +func annotateChanges(ctx context.Context, workspacePath string, ignore *patch.IgnoreSet) ([]git.FileChange, error) { + changes, err := git.StatusPorcelain(ctx, workspacePath, nil) + if err != nil { + return nil, err + } + kept := changes[:0] + for _, change := range changes { + if change.Status == "??" && ignore.Match(change.Path) { + continue + } + kept = append(kept, change) + } + return kept, nil } func modifiedFeatureFiles(changes []git.FileChange, feature FeatureSpec) annotateFileSet { diff --git a/packages/browseros/tools/patch/internal/engine/apply.go b/packages/browseros/tools/patch/internal/engine/apply.go index 7143f65442..0890c857b6 100644 --- a/packages/browseros/tools/patch/internal/engine/apply.go +++ b/packages/browseros/tools/patch/internal/engine/apply.go @@ -27,7 +27,11 @@ type ApplyOptions struct { // RestorePendingStash carries a rebase-mode sync's intent through a // conflict pause: continue/skip restore the parked stash on completion. RestorePendingStash bool - Progress Progress + // AutoAnnotate creates feature commits once the apply completes without + // conflicts. Like RestorePendingStash, the intent survives a conflict + // pause so continue/skip finish with the same annotation. + AutoAnnotate bool + Progress Progress } type ApplyResult struct { @@ -42,6 +46,7 @@ type ApplyResult struct { StashConflict bool `json:"stash_conflict,omitempty"` StashConflictFiles []string `json:"stash_conflict_files,omitempty"` Conflicts []resolve.Operation `json:"conflicts,omitempty"` + Annotate *AnnotateResult `json:"annotate,omitempty"` } func Apply(ctx context.Context, opts ApplyOptions) (*ApplyResult, error) { @@ -63,28 +68,46 @@ func Apply(ctx context.Context, opts ApplyOptions) (*ApplyResult, error) { Orphaned: orphaned, } if len(ops) == 0 { - if err := markApplyComplete(opts.Workspace.Path, opts.Repo.BaseCommit, repoRev); err != nil { - return nil, err - } - if err := clearResolveState(opts.Workspace.Path); err != nil { + if err := finishApply(ctx, opts, repoRev, result); err != nil { return nil, err } return result, nil } - next, err := applyOperationRange(ctx, opts.Workspace, opts.Repo, ops, 0, nil, nil, opts.RestorePendingStash, result, opts.Progress) + next, err := applyOperationRange(ctx, opts.Workspace, opts.Repo, ops, 0, nil, nil, opts.RestorePendingStash, opts.AutoAnnotate, result, opts.Progress) if err != nil { return nil, err } if next < len(ops) { return result, nil } - if err := markApplyComplete(opts.Workspace.Path, opts.Repo.BaseCommit, repoRev); err != nil { + if err := finishApply(ctx, opts, repoRev, result); err != nil { return nil, err } + return result, nil +} + +// finishApply records completion state and, when requested, turns the applied +// changes into feature commits in the same run. +func finishApply(ctx context.Context, opts ApplyOptions, repoRev string, result *ApplyResult) error { + if err := markApplyComplete(opts.Workspace.Path, opts.Repo.BaseCommit, repoRev); err != nil { + return err + } if err := clearResolveState(opts.Workspace.Path); err != nil { - return nil, err + return err } - return result, nil + if !opts.AutoAnnotate { + return nil + } + annotateResult, err := Annotate(ctx, AnnotateOptions{ + Workspace: opts.Workspace, + Repo: opts.Repo, + Progress: opts.Progress, + }) + if err != nil { + return fmt.Errorf("patches applied, but annotate failed: %w", err) + } + result.Annotate = annotateResult + return nil } type ContinueOptions struct { @@ -120,22 +143,14 @@ func Continue(ctx context.Context, opts ContinueOptions) (*ApplyResult, error) { Conflicts: nil, } reportProgress(opts.Progress, "Continuing patch resolution") - next, err := applyOperationRange(ctx, ws, repoInfo, state.Operations, state.Current+1, state.Resolved, state.Skipped, state.RestorePendingStash, result, opts.Progress) + next, err := applyOperationRange(ctx, ws, repoInfo, state.Operations, state.Current+1, state.Resolved, state.Skipped, state.RestorePendingStash, state.AutoAnnotate, result, opts.Progress) if err != nil { return nil, err } if next >= len(state.Operations) && len(result.Conflicts) == 0 { - if err := markApplyComplete(ws.Path, state.BaseCommit, state.RepoRev); err != nil { - return nil, err - } - if err := resolve.Delete(ws.Path); err != nil { + if err := finishResolvedRun(ctx, ws, repoInfo, state, result, opts.Progress); err != nil { return nil, err } - if state.RestorePendingStash { - if err := restorePendingStash(ctx, ws.Path, result, opts.Progress); err != nil { - return nil, err - } - } } return result, nil } @@ -169,24 +184,47 @@ func Skip(ctx context.Context, opts SkipOptions) (*ApplyResult, error) { Applied: append([]string{}, state.Resolved...), } reportProgress(opts.Progress, "Skipping current conflict") - next, err := applyOperationRange(ctx, ws, repoInfo, state.Operations, state.Current+1, state.Resolved, state.Skipped, state.RestorePendingStash, result, opts.Progress) + next, err := applyOperationRange(ctx, ws, repoInfo, state.Operations, state.Current+1, state.Resolved, state.Skipped, state.RestorePendingStash, state.AutoAnnotate, result, opts.Progress) if err != nil { return nil, err } if next >= len(state.Operations) && len(result.Conflicts) == 0 { - if err := markApplyComplete(ws.Path, state.BaseCommit, state.RepoRev); err != nil { + if err := finishResolvedRun(ctx, ws, repoInfo, state, result, opts.Progress); err != nil { return nil, err } - if err := resolve.Delete(ws.Path); err != nil { - return nil, err + } + return result, nil +} + +// finishResolvedRun closes out a conflict loop that reached the last +// operation: record completion, drop the resolve state, then honor the +// original run's intent (feature commits for an apply, stash restore for a +// rebase-mode sync). Annotate runs before the stash restore so restored local +// changes are never swept into feature commits. +func finishResolvedRun(ctx context.Context, ws workspace.Entry, repoInfo *repo.Info, state *resolve.State, result *ApplyResult, progress Progress) error { + if err := markApplyComplete(ws.Path, state.BaseCommit, state.RepoRev); err != nil { + return err + } + if err := resolve.Delete(ws.Path); err != nil { + return err + } + if state.AutoAnnotate { + annotateResult, err := Annotate(ctx, AnnotateOptions{ + Workspace: ws, + Repo: repoInfo, + Progress: progress, + }) + if err != nil { + return fmt.Errorf("patches applied, but annotate failed: %w", err) } - if state.RestorePendingStash { - if err := restorePendingStash(ctx, ws.Path, result, opts.Progress); err != nil { - return nil, err - } + result.Annotate = annotateResult + } + if state.RestorePendingStash { + if err := restorePendingStash(ctx, ws.Path, result, progress); err != nil { + return err } } - return result, nil + return nil } // restorePendingStash pops a stash a conflicted sync left behind once the @@ -323,6 +361,7 @@ func applyOperationRange( resolved []string, skipped []string, restorePending bool, + autoAnnotate bool, result *ApplyResult, progress Progress, ) (int, error) { @@ -359,6 +398,7 @@ func applyOperationRange( Resolved: append([]string{}, resolved...), Skipped: append([]string{}, skipped...), RestorePendingStash: restorePending, + AutoAnnotate: autoAnnotate, } if err := resolve.Save(ws.Path, state); err != nil { return idx, err diff --git a/packages/browseros/tools/patch/internal/engine/engine_test.go b/packages/browseros/tools/patch/internal/engine/engine_test.go index be66fed177..14975e1dbf 100644 --- a/packages/browseros/tools/patch/internal/engine/engine_test.go +++ b/packages/browseros/tools/patch/internal/engine/engine_test.go @@ -584,6 +584,254 @@ func TestApplyReportsPatchProgress(t *testing.T) { assertFile(t, filepath.Join(workspacePath, "chrome", "browser.cc"), "patched\n") } +func TestApplyAutoAnnotatesAfterCleanApply(t *testing.T) { + ctx := context.Background() + workspacePath := initGitRepo(t) + writeFile(t, filepath.Join(workspacePath, "chrome", "core.cc"), "base\n") + runGit(t, workspacePath, "add", "chrome/core.cc") + runGit(t, workspacePath, "commit", "-m", "workspace base") + baseCommit := gitOutput(t, workspacePath, "rev-parse", "HEAD") + + repoInfo := newPatchRepo(t, baseCommit) + writePatchFromEdit(t, ctx, workspacePath, repoInfo, baseCommit, "chrome/core.cc", "patched\n") + writeFeaturesYAML(t, repoInfo.Root, `version: "1.0" +features: + browseros-core: + description: "chore: browseros core" + files: + - chrome/core.cc +`) + + result, err := Apply(ctx, ApplyOptions{ + Workspace: workspace.Entry{Name: "ws", Path: workspacePath}, + Repo: repoInfo, + AutoAnnotate: true, + }) + if err != nil { + t.Fatalf("Apply: %v", err) + } + if result.Annotate == nil || result.Annotate.CommitsCreated != 1 { + t.Fatalf("expected one auto-annotate commit, got %+v", result.Annotate) + } + if len(result.Annotate.Unclaimed) != 0 { + t.Fatalf("expected no unclaimed changes, got %v", result.Annotate.Unclaimed) + } + if subject := gitOutput(t, workspacePath, "log", "-1", "--format=%s"); subject != "chore: browseros core" { + t.Fatalf("unexpected HEAD subject %q", subject) + } + if status := gitOutput(t, workspacePath, "status", "--porcelain", "--", "chrome"); status != "" { + t.Fatalf("expected clean checkout, got %q", status) + } +} + +func TestApplyWithNoOpsStillAutoAnnotates(t *testing.T) { + ctx := context.Background() + workspacePath := initGitRepo(t) + writeFile(t, filepath.Join(workspacePath, "chrome", "core.cc"), "base\n") + runGit(t, workspacePath, "add", "chrome/core.cc") + runGit(t, workspacePath, "commit", "-m", "workspace base") + baseCommit := gitOutput(t, workspacePath, "rev-parse", "HEAD") + + repoInfo := newPatchRepo(t, baseCommit) + writePatchFromEdit(t, ctx, workspacePath, repoInfo, baseCommit, "chrome/core.cc", "patched\n") + writeFeaturesYAML(t, repoInfo.Root, `version: "1.0" +features: + browseros-core: + description: "chore: browseros core" + files: + - chrome/core.cc +`) + + ws := workspace.Entry{Name: "ws", Path: workspacePath} + if _, err := Apply(ctx, ApplyOptions{Workspace: ws, Repo: repoInfo}); err != nil { + t.Fatalf("Apply without annotate: %v", err) + } + result, err := Apply(ctx, ApplyOptions{Workspace: ws, Repo: repoInfo, AutoAnnotate: true}) + if err != nil { + t.Fatalf("Apply with annotate: %v", err) + } + if len(result.Applied) != 0 { + t.Fatalf("expected up-to-date tree to need no operations, got %v", result.Applied) + } + if result.Annotate == nil || result.Annotate.CommitsCreated != 1 { + t.Fatalf("expected annotate to commit leftover applied changes, got %+v", result.Annotate) + } +} + +func TestAnnotateRefusesDuringConflictResolution(t *testing.T) { + ctx := context.Background() + workspacePath := initGitRepo(t) + writeFile(t, filepath.Join(workspacePath, "chrome", "core.cc"), "base\n") + runGit(t, workspacePath, "add", "chrome/core.cc") + runGit(t, workspacePath, "commit", "-m", "workspace base") + baseCommit := gitOutput(t, workspacePath, "rev-parse", "HEAD") + + repoInfo := newPatchRepo(t, baseCommit) + writeFeaturesYAML(t, repoInfo.Root, `version: "1.0" +features: + browseros-core: + description: "chore: browseros core" + files: + - chrome/ +`) + if err := resolve.Save(workspacePath, &resolve.State{ + Workspace: workspacePath, + RepoRoot: repoInfo.Root, + BaseCommit: baseCommit, + Operations: []resolve.Operation{{ChromiumPath: "chrome/core.cc", PatchRel: "chrome/core.cc", Op: patch.OpModify}}, + }); err != nil { + t.Fatalf("resolve.Save: %v", err) + } + + _, err := Annotate(ctx, AnnotateOptions{ + Workspace: workspace.Entry{Name: "ws", Path: workspacePath}, + Repo: repoInfo, + }) + if err == nil || !strings.Contains(err.Error(), "conflict resolution is in progress") { + t.Fatalf("expected pending-conflict refusal, got %v", err) + } +} + +func TestAnnotateSkipsUntrackedJunk(t *testing.T) { + ctx := context.Background() + workspacePath := initGitRepo(t) + writeFile(t, filepath.Join(workspacePath, "chrome", "core.cc"), "base\n") + runGit(t, workspacePath, "add", "chrome/core.cc") + runGit(t, workspacePath, "commit", "-m", "workspace base") + baseCommit := gitOutput(t, workspacePath, "rev-parse", "HEAD") + + writeFile(t, filepath.Join(workspacePath, "chrome", "core.cc"), "patched\n") + writeFile(t, filepath.Join(workspacePath, "chrome", "core.cc.rej"), "reject junk\n") + writeFile(t, filepath.Join(workspacePath, "chrome", "core.cc.orig"), "orig junk\n") + writeFile(t, filepath.Join(workspacePath, "chrome", "debug.log"), "log junk\n") + + repoInfo := newPatchRepo(t, baseCommit) + writeFeaturesYAML(t, repoInfo.Root, `version: "1.0" +features: + browseros-core: + description: "chore: browseros core" + files: + - chrome/ +`) + + result, err := Annotate(ctx, AnnotateOptions{ + Workspace: workspace.Entry{Name: "ws", Path: workspacePath}, + Repo: repoInfo, + }) + if err != nil { + t.Fatalf("Annotate: %v", err) + } + if result.CommitsCreated != 1 { + t.Fatalf("expected one commit, got %+v", result) + } + if !slices.Equal(result.Committed[0].Files, []string{"chrome/core.cc"}) { + t.Fatalf("junk must stay out of feature commits, got %v", result.Committed[0].Files) + } + if len(result.Unclaimed) != 0 { + t.Fatalf("junk must not be reported as unclaimed, got %v", result.Unclaimed) + } + if files := gitOutput(t, workspacePath, "show", "--name-only", "--format=", "HEAD"); files != "chrome/core.cc" { + t.Fatalf("commit files = %q", files) + } +} + +func TestAnnotateReportsUnclaimedChanges(t *testing.T) { + ctx := context.Background() + workspacePath := initGitRepo(t) + writeFile(t, filepath.Join(workspacePath, "chrome", "core.cc"), "base\n") + writeFile(t, filepath.Join(workspacePath, "content", "render.cc"), "base\n") + runGit(t, workspacePath, "add", "chrome", "content") + runGit(t, workspacePath, "commit", "-m", "workspace base") + baseCommit := gitOutput(t, workspacePath, "rev-parse", "HEAD") + + writeFile(t, filepath.Join(workspacePath, "chrome", "core.cc"), "patched\n") + writeFile(t, filepath.Join(workspacePath, "content", "render.cc"), "patched\n") + + repoInfo := newPatchRepo(t, baseCommit) + writeFeaturesYAML(t, repoInfo.Root, `version: "1.0" +features: + browseros-core: + description: "chore: browseros core" + files: + - chrome/ +`) + + result, err := Annotate(ctx, AnnotateOptions{ + Workspace: workspace.Entry{Name: "ws", Path: workspacePath}, + Repo: repoInfo, + }) + if err != nil { + t.Fatalf("Annotate: %v", err) + } + if result.CommitsCreated != 1 { + t.Fatalf("expected the claimed file committed, got %+v", result) + } + if !slices.Equal(result.Unclaimed, []string{"content/render.cc"}) { + t.Fatalf("expected unclaimed report, got %v", result.Unclaimed) + } + if status := gitOutput(t, workspacePath, "status", "--porcelain", "--", "content"); status != "M content/render.cc" { + t.Fatalf("unclaimed file should stay uncommitted, got %q", status) + } +} + +func TestSkipCompletesAutoAnnotateFromPausedApply(t *testing.T) { + ctx := context.Background() + workspacePath := initGitRepo(t) + writeFile(t, filepath.Join(workspacePath, "chrome", "aaa.cc"), "base-aaa\n") + writeFile(t, filepath.Join(workspacePath, "chrome", "bbb.cc"), "base-bbb\n") + runGit(t, workspacePath, "add", "chrome") + runGit(t, workspacePath, "commit", "-m", "workspace base") + baseCommit := gitOutput(t, workspacePath, "rev-parse", "HEAD") + + repoInfo := newPatchRepo(t, baseCommit) + // aaa.cc's patch context does not exist at base -> genuine conflict pause. + writeFile(t, filepath.Join(repoInfo.PatchesDir, "chrome", "aaa.cc"), testPatchContent("chrome/aaa.cc", "mismatched-context", "patched-aaa")) + writePatchFromEdit(t, ctx, workspacePath, repoInfo, baseCommit, "chrome/bbb.cc", "patched-bbb\n") + writeFeaturesYAML(t, repoInfo.Root, `version: "1.0" +features: + browseros-core: + description: "chore: browseros core" + files: + - chrome/ +`) + + ws := workspace.Entry{Name: "ws", Path: workspacePath} + paused, err := Apply(ctx, ApplyOptions{Workspace: ws, Repo: repoInfo, AutoAnnotate: true}) + if err != nil { + t.Fatalf("Apply: %v", err) + } + if len(paused.Conflicts) != 1 || paused.Conflicts[0].ChromiumPath != "chrome/aaa.cc" { + t.Fatalf("expected pause on chrome/aaa.cc, got %+v", paused.Conflicts) + } + if paused.Annotate != nil { + t.Fatalf("paused apply must not annotate a half-applied tree") + } + state, err := resolve.Load(workspacePath) + if err != nil { + t.Fatalf("resolve.Load: %v", err) + } + if !state.AutoAnnotate { + t.Fatalf("expected auto-annotate intent to survive the pause") + } + + result, err := Skip(ctx, SkipOptions{Workspace: ws}) + if err != nil { + t.Fatalf("Skip: %v", err) + } + if result.Annotate == nil || result.Annotate.CommitsCreated != 1 { + t.Fatalf("expected skip to finish the requested annotation, got %+v", result.Annotate) + } + if !slices.Equal(result.Annotate.Committed[0].Files, []string{"chrome/bbb.cc"}) { + t.Fatalf("expected only the applied file committed, got %v", result.Annotate.Committed[0].Files) + } + if resolve.Exists(workspacePath) { + t.Fatalf("expected resolve state cleared after skip completion") + } + if files := gitOutput(t, workspacePath, "show", "--name-only", "--format=", "HEAD"); files != "chrome/bbb.cc" { + t.Fatalf("reject junk must stay out of the feature commit, got %q", files) + } +} + func TestInspectWorkspaceSkipsIgnoredUntrackedFiles(t *testing.T) { ctx := context.Background() workspacePath := initGitRepo(t) diff --git a/packages/browseros/tools/patch/internal/resolve/resolve.go b/packages/browseros/tools/patch/internal/resolve/resolve.go index b5ef091abd..018c57ad73 100644 --- a/packages/browseros/tools/patch/internal/resolve/resolve.go +++ b/packages/browseros/tools/patch/internal/resolve/resolve.go @@ -33,6 +33,9 @@ type State struct { // sync: when the conflict loop completes, the parked stash comes back. // Stashes parked explicitly with --no-rebase stay parked. RestorePendingStash bool `json:"restore_pending_stash,omitempty"` + // AutoAnnotate marks that the paused apply wants feature commits once the + // conflict loop completes, so continue/skip finish what apply started. + AutoAnnotate bool `json:"auto_annotate,omitempty"` } func Path(workspacePath string) string { From 38578df24b4c51331b115b4ed49654914e6558cb Mon Sep 17 00:00:00 2001 From: Nikhil Sonti Date: Thu, 2 Jul 2026 14:59:52 -0700 Subject: [PATCH 2/4] fix(patches): harden auto-annotate against review findings - apply and sync refuse to start while a conflict resolution is pending, instead of silently clobbering the paused state and bypassing the annotate guard - continue/skip completion excludes skipped conflicts from annotation: the --reject fallback can leave half-applied hunks in tracked files - --changed applies no longer auto-annotate (surgical, like -- files) - a repo without a features registry skips annotation instead of failing the apply - annotate failures are recorded on the result (annotate_error) so the apply outcome is never discarded; commands render it and exit 1 with a standalone-annotate hint - annotate reuses one status snapshot, rescanning only after a commit (untracked-files=all scans are seconds on a Chromium checkout) - dedupe completion + report-path helpers --- packages/browseros/tools/patch/cmd/apply.go | 15 +-- packages/browseros/tools/patch/cmd/common.go | 33 +++++- .../browseros/tools/patch/cmd/continue.go | 8 +- packages/browseros/tools/patch/cmd/skip.go | 8 +- .../tools/patch/internal/engine/annotate.go | 81 +++++++------ .../tools/patch/internal/engine/apply.go | 57 ++++++--- .../patch/internal/engine/engine_test.go | 108 +++++++++++++++++- .../tools/patch/internal/engine/sync.go | 3 + 8 files changed, 237 insertions(+), 76 deletions(-) diff --git a/packages/browseros/tools/patch/cmd/apply.go b/packages/browseros/tools/patch/cmd/apply.go index 82e69ef182..a18f3bee18 100644 --- a/packages/browseros/tools/patch/cmd/apply.go +++ b/packages/browseros/tools/patch/cmd/apply.go @@ -43,9 +43,10 @@ func init() { ChangedRef: changed, RangeEnd: rangeEnd, Filters: filters, - // A filtered apply is surgical; a full annotate would sweep - // changes outside the requested files into feature commits. - AutoAnnotate: !noAnnotate && len(filters) == 0, + // Filtered and --changed applies are surgical; a full + // annotate would sweep changes outside the requested scope + // into feature commits. + AutoAnnotate: !noAnnotate && len(filters) == 0 && changed == "", Progress: commandProgress(cmd), }) if err != nil { @@ -63,13 +64,13 @@ func init() { } fmt.Println(ui.Hint(`Run "browseros-patch continue" after fixing the current conflict.`)) } - if result.Annotate != nil { - fmt.Println() - printAnnotateResult(ws, result.Annotate) - } + printAnnotateOutcome(ws, result) }); err != nil { return err } + if err := annotateFailureExit(result); err != nil { + return err + } return conflictPauseError(len(result.Conflicts) > 0) }, } diff --git a/packages/browseros/tools/patch/cmd/common.go b/packages/browseros/tools/patch/cmd/common.go index 0b54f0e8de..41f340aee7 100644 --- a/packages/browseros/tools/patch/cmd/common.go +++ b/packages/browseros/tools/patch/cmd/common.go @@ -82,11 +82,12 @@ Capture flow (turn checkout changes into patches): 3. browseros-patch publish -m "chore: sync patches" # commit + push chromium_patches Feature commit flow (turn checkout changes into feature commits): -1. browseros-patch apply ch1 annotates automatically after a conflict-free apply; pass --no-annotate to skip. - A filtered apply (-- files...) never auto-annotates. continue/skip finish the annotation a paused apply asked for. +1. browseros-patch apply ch1 # auto-annotates after a conflict-free apply (--no-annotate to skip) 2. browseros-patch annotate ch1 # standalone: commit changed files for all bos_build/features.yaml features -3. annotate refuses to run while a conflict resolution is pending; finish continue/skip/abort first. -4. Changed files no feature claims are reported as "unclaimed" and left uncommitted; claim them in bos_build/features.yaml. +Annotation rules: +- Filtered (-- files...) and --changed applies never auto-annotate; continue/skip finish a paused apply's annotation, excluding skipped conflicts. +- apply, sync, and annotate refuse to start while a conflict resolution is pending; finish continue/skip/abort first. +- Changes no feature claims are reported as "unclaimed" and left uncommitted; claim them in bos_build/features.yaml. Pool refresh flow (rebuild browseros branch before leasing a checkout): 1. browseros-patch status ch1 --json # check patches_freshness @@ -134,6 +135,30 @@ func ensureRepoConfigured(override string) error { return nil } +// printAnnotateOutcome renders the auto-annotate section of an apply-family +// result: the feature-commit summary on success, a recovery hint on failure. +func printAnnotateOutcome(ws workspace.Entry, result *engine.ApplyResult) { + if result.Annotate != nil { + fmt.Println() + printAnnotateResult(ws, result.Annotate) + } + if result.AnnotateError != "" { + fmt.Println(ui.Warning("Patches applied, but annotate failed")) + fmt.Printf(" %s\n", result.AnnotateError) + fmt.Println(ui.Hint(fmt.Sprintf(`Fix the cause, then run "browseros-patch annotate %s".`, ws.Name))) + } +} + +// annotateFailureExit maps an auto-annotate failure to exit 1 after rendering, +// so scripts notice the missing feature commits while the apply outcome stays +// visible in the output. +func annotateFailureExit(result *engine.ApplyResult) error { + if result.AnnotateError != "" { + return &exitCodeError{code: 1} + } + return nil +} + // printStashOutcome reports what happened to a pending sync stash once the // conflict loop completes. func printStashOutcome(result *engine.ApplyResult) { diff --git a/packages/browseros/tools/patch/cmd/continue.go b/packages/browseros/tools/patch/cmd/continue.go index 36e860666c..59b74328ee 100644 --- a/packages/browseros/tools/patch/cmd/continue.go +++ b/packages/browseros/tools/patch/cmd/continue.go @@ -35,14 +35,14 @@ func init() { fmt.Printf(" %s\n", conflict.ChromiumPath) } } - if result.Annotate != nil { - fmt.Println() - printAnnotateResult(ws, result.Annotate) - } + printAnnotateOutcome(ws, result) printStashOutcome(result) }); err != nil { return err } + if err := annotateFailureExit(result); err != nil { + return err + } return conflictPauseError(len(result.Conflicts) > 0 || result.StashConflict) }, } diff --git a/packages/browseros/tools/patch/cmd/skip.go b/packages/browseros/tools/patch/cmd/skip.go index 741c616e71..ce9fa3e357 100644 --- a/packages/browseros/tools/patch/cmd/skip.go +++ b/packages/browseros/tools/patch/cmd/skip.go @@ -35,14 +35,14 @@ func init() { fmt.Printf(" %s\n", conflict.ChromiumPath) } } - if result.Annotate != nil { - fmt.Println() - printAnnotateResult(ws, result.Annotate) - } + printAnnotateOutcome(ws, result) printStashOutcome(result) }); err != nil { return err } + if err := annotateFailureExit(result); err != nil { + return err + } return conflictPauseError(len(result.Conflicts) > 0 || result.StashConflict) }, } diff --git a/packages/browseros/tools/patch/internal/engine/annotate.go b/packages/browseros/tools/patch/internal/engine/annotate.go index 761b54202f..3d1de3bbbc 100644 --- a/packages/browseros/tools/patch/internal/engine/annotate.go +++ b/packages/browseros/tools/patch/internal/engine/annotate.go @@ -16,7 +16,10 @@ import ( type AnnotateOptions struct { Workspace workspace.Entry Repo *repo.Info - Progress Progress + // Exclude lists checkout paths deliberately left uncommitted — skipped + // conflicts whose files may hold partially applied hunks. + Exclude []string + Progress Progress } type AnnotateResult struct { @@ -74,6 +77,15 @@ func Annotate(ctx context.Context, opts AnnotateOptions) (*AnnotateResult, error Committed: []AnnotateCommittedFeature{}, Skipped: []AnnotateSkippedFeature{}, } + // One status snapshot serves consecutive features; a full scan is seconds + // on a Chromium checkout. Rescan only after a commit changes what is + // dirty, and once more before the unclaimed report if staging may have + // normalized entries without committing. + changes, err := annotateChanges(ctx, opts.Workspace.Path, ignore, opts.Exclude) + if err != nil { + return nil, err + } + staged := false for _, feature := range features { result.Processed++ reportProgress(opts.Progress, "Annotating feature %s", feature.Name) @@ -86,10 +98,6 @@ func Annotate(ctx context.Context, opts AnnotateOptions) (*AnnotateResult, error result.FeaturesSkipped++ continue } - changes, err := annotateChanges(ctx, opts.Workspace.Path, ignore) - if err != nil { - return nil, err - } files := modifiedFeatureFiles(changes, feature) if len(files.report) == 0 { result.Skipped = append(result.Skipped, AnnotateSkippedFeature{ @@ -104,6 +112,7 @@ func Annotate(ctx context.Context, opts AnnotateOptions) (*AnnotateResult, error if err != nil { return nil, fmt.Errorf("commit feature %s: %w", feature.Name, err) } + staged = true if !committed { result.Skipped = append(result.Skipped, AnnotateSkippedFeature{ Name: feature.Name, @@ -120,31 +129,18 @@ func Annotate(ctx context.Context, opts AnnotateOptions) (*AnnotateResult, error Files: committedFiles, }) result.CommitsCreated++ + if changes, err = annotateChanges(ctx, opts.Workspace.Path, ignore, opts.Exclude); err != nil { + return nil, err + } + staged = false } - unclaimed, err := unclaimedChanges(ctx, opts.Workspace.Path, ignore) - if err != nil { - return nil, err - } - result.Unclaimed = unclaimed - return result, nil -} - -// unclaimedChanges lists what is still dirty after every feature committed its -// files — changes no feature in the registry claims. -func unclaimedChanges(ctx context.Context, workspacePath string, ignore *patch.IgnoreSet) ([]string, error) { - changes, err := annotateChanges(ctx, workspacePath, ignore) - if err != nil { - return nil, err - } - seen := map[string]bool{} - var paths []string - for _, change := range changes { - for _, rel := range changeReportPaths(change) { - appendUniquePath(&paths, seen, rel) + if staged { + if changes, err = annotateChanges(ctx, opts.Workspace.Path, ignore, opts.Exclude); err != nil { + return nil, err } } - slices.Sort(paths) - return paths, nil + result.Unclaimed = uniqueReportPaths(changes) + return result, nil } func mappingValue(node *yaml.Node, key string) *yaml.Node { @@ -193,8 +189,9 @@ func stringSequence(node *yaml.Node, key string) []string { // annotateChanges returns working-tree changes eligible for feature commits. // Untracked junk (reject files, logs, .browseros-patchignore patterns) is -// filtered like extract does; tracked modifications always pass through. -func annotateChanges(ctx context.Context, workspacePath string, ignore *patch.IgnoreSet) ([]git.FileChange, error) { +// filtered like extract does; tracked modifications always pass through +// unless explicitly excluded. +func annotateChanges(ctx context.Context, workspacePath string, ignore *patch.IgnoreSet, exclude []string) ([]git.FileChange, error) { changes, err := git.StatusPorcelain(ctx, workspacePath, nil) if err != nil { return nil, err @@ -204,11 +201,27 @@ func annotateChanges(ctx context.Context, workspacePath string, ignore *patch.Ig if change.Status == "??" && ignore.Match(change.Path) { continue } + if len(exclude) > 0 && patch.PathMatches(patch.NormalizeChromiumPath(change.Path), exclude) { + continue + } kept = append(kept, change) } return kept, nil } +// uniqueReportPaths flattens changes into sorted, deduplicated checkout paths. +func uniqueReportPaths(changes []git.FileChange) []string { + seen := map[string]bool{} + var paths []string + for _, change := range changes { + for _, rel := range changeReportPaths(change) { + appendUniquePath(&paths, seen, rel) + } + } + slices.Sort(paths) + return paths +} + func modifiedFeatureFiles(changes []git.FileChange, feature FeatureSpec) annotateFileSet { set := annotateFileSet{} reportSeen := map[string]bool{} @@ -318,13 +331,5 @@ func committedFeatureFiles(ctx context.Context, workspacePath string, commit str if err != nil { return nil, err } - seen := map[string]bool{} - var paths []string - for _, change := range changes { - for _, rel := range changeReportPaths(change) { - appendUniquePath(&paths, seen, rel) - } - } - slices.Sort(paths) - return paths, nil + return uniqueReportPaths(changes), nil } diff --git a/packages/browseros/tools/patch/internal/engine/apply.go b/packages/browseros/tools/patch/internal/engine/apply.go index 0890c857b6..f868b38a76 100644 --- a/packages/browseros/tools/patch/internal/engine/apply.go +++ b/packages/browseros/tools/patch/internal/engine/apply.go @@ -47,9 +47,16 @@ type ApplyResult struct { StashConflictFiles []string `json:"stash_conflict_files,omitempty"` Conflicts []resolve.Operation `json:"conflicts,omitempty"` Annotate *AnnotateResult `json:"annotate,omitempty"` + // AnnotateError reports an auto-annotate failure without discarding the + // apply outcome: the patches landed and the resolve state is already + // settled, so the recovery is a standalone annotate, not a retry. + AnnotateError string `json:"annotate_error,omitempty"` } func Apply(ctx context.Context, opts ApplyOptions) (*ApplyResult, error) { + if err := requireNoPendingResolution(opts.Workspace); err != nil { + return nil, err + } repoRev, err := git.HeadRev(ctx, opts.Repo.Root) if err != nil { return nil, err @@ -86,6 +93,18 @@ func Apply(ctx context.Context, opts ApplyOptions) (*ApplyResult, error) { return result, nil } +// requireNoPendingResolution stops a new run from clobbering a paused +// conflict resolution: the old resolve state would be silently overwritten or +// deleted, stranding continue/skip and losing the run's persisted intent. +func requireNoPendingResolution(ws workspace.Entry) error { + if !resolve.Exists(ws.Path) { + return nil + } + return fmt.Errorf( + "conflict resolution is in progress for %s; finish it with \"browseros-patch continue\", \"browseros-patch skip\", or \"browseros-patch abort\" first", + ws.Name) +} + // finishApply records completion state and, when requested, turns the applied // changes into feature commits in the same run. func finishApply(ctx context.Context, opts ApplyOptions, repoRev string, result *ApplyResult) error { @@ -98,16 +117,30 @@ func finishApply(ctx context.Context, opts ApplyOptions, repoRev string, result if !opts.AutoAnnotate { return nil } + annotateApplied(ctx, opts.Workspace, opts.Repo, nil, result, opts.Progress) + return nil +} + +// annotateApplied runs the auto-annotate step of a completed apply. A repo +// without a features registry skips quietly, and a failure is recorded on the +// result instead of returned: the apply itself succeeded and its state is +// already settled, so the caller must still see what was applied. +func annotateApplied(ctx context.Context, ws workspace.Entry, repoInfo *repo.Info, exclude []string, result *ApplyResult, progress Progress) { + if _, err := FeatureFilePath(repoInfo.Root); err != nil { + reportProgress(progress, "No features registry; skipping annotation") + return + } annotateResult, err := Annotate(ctx, AnnotateOptions{ - Workspace: opts.Workspace, - Repo: opts.Repo, - Progress: opts.Progress, + Workspace: ws, + Repo: repoInfo, + Exclude: exclude, + Progress: progress, }) if err != nil { - return fmt.Errorf("patches applied, but annotate failed: %w", err) + result.AnnotateError = err.Error() + return } result.Annotate = annotateResult - return nil } type ContinueOptions struct { @@ -200,7 +233,9 @@ func Skip(ctx context.Context, opts SkipOptions) (*ApplyResult, error) { // operation: record completion, drop the resolve state, then honor the // original run's intent (feature commits for an apply, stash restore for a // rebase-mode sync). Annotate runs before the stash restore so restored local -// changes are never swept into feature commits. +// changes are never swept into feature commits, and skipped conflicts are +// excluded — their files may hold partially applied hunks the user still has +// to reconcile. func finishResolvedRun(ctx context.Context, ws workspace.Entry, repoInfo *repo.Info, state *resolve.State, result *ApplyResult, progress Progress) error { if err := markApplyComplete(ws.Path, state.BaseCommit, state.RepoRev); err != nil { return err @@ -209,15 +244,7 @@ func finishResolvedRun(ctx context.Context, ws workspace.Entry, repoInfo *repo.I return err } if state.AutoAnnotate { - annotateResult, err := Annotate(ctx, AnnotateOptions{ - Workspace: ws, - Repo: repoInfo, - Progress: progress, - }) - if err != nil { - return fmt.Errorf("patches applied, but annotate failed: %w", err) - } - result.Annotate = annotateResult + annotateApplied(ctx, ws, repoInfo, state.Skipped, result, progress) } if state.RestorePendingStash { if err := restorePendingStash(ctx, ws.Path, result, progress); err != nil { diff --git a/packages/browseros/tools/patch/internal/engine/engine_test.go b/packages/browseros/tools/patch/internal/engine/engine_test.go index 14975e1dbf..d2ac2015c8 100644 --- a/packages/browseros/tools/patch/internal/engine/engine_test.go +++ b/packages/browseros/tools/patch/internal/engine/engine_test.go @@ -777,15 +777,22 @@ features: func TestSkipCompletesAutoAnnotateFromPausedApply(t *testing.T) { ctx := context.Background() workspacePath := initGitRepo(t) - writeFile(t, filepath.Join(workspacePath, "chrome", "aaa.cc"), "base-aaa\n") + writeFile(t, filepath.Join(workspacePath, "chrome", "aaa.cc"), "a1\na2\na3\na4\na5\na6\na7\na8\na9\na10\n") writeFile(t, filepath.Join(workspacePath, "chrome", "bbb.cc"), "base-bbb\n") runGit(t, workspacePath, "add", "chrome") runGit(t, workspacePath, "commit", "-m", "workspace base") baseCommit := gitOutput(t, workspacePath, "rev-parse", "HEAD") repoInfo := newPatchRepo(t, baseCommit) - // aaa.cc's patch context does not exist at base -> genuine conflict pause. - writeFile(t, filepath.Join(repoInfo.PatchesDir, "chrome", "aaa.cc"), testPatchContent("chrome/aaa.cc", "mismatched-context", "patched-aaa")) + // Two hunks: the first applies at base, the second's context does not + // exist -> the --reject fallback half-applies the file and pauses. + writeFile(t, filepath.Join(repoInfo.PatchesDir, "chrome", "aaa.cc"), + "diff --git a/chrome/aaa.cc b/chrome/aaa.cc\n"+ + "index 0000000000000000000000000000000000000000..1111111111111111111111111111111111111111 100644\n"+ + "--- a/chrome/aaa.cc\n"+ + "+++ b/chrome/aaa.cc\n"+ + "@@ -1,5 +1,5 @@\n a1\n-a2\n+a2 patched\n a3\n a4\n a5\n"+ + "@@ -7,4 +7,4 @@\n a7\n-mismatched-context\n+a8 patched\n a9\n a10\n") writePatchFromEdit(t, ctx, workspacePath, repoInfo, baseCommit, "chrome/bbb.cc", "patched-bbb\n") writeFeaturesYAML(t, repoInfo.Root, `version: "1.0" features: @@ -806,6 +813,7 @@ features: if paused.Annotate != nil { t.Fatalf("paused apply must not annotate a half-applied tree") } + assertFile(t, filepath.Join(workspacePath, "chrome", "aaa.cc"), "a1\na2 patched\na3\na4\na5\na6\na7\na8\na9\na10\n") state, err := resolve.Load(workspacePath) if err != nil { t.Fatalf("resolve.Load: %v", err) @@ -824,12 +832,104 @@ features: if !slices.Equal(result.Annotate.Committed[0].Files, []string{"chrome/bbb.cc"}) { t.Fatalf("expected only the applied file committed, got %v", result.Annotate.Committed[0].Files) } + if slices.Contains(result.Annotate.Unclaimed, "chrome/aaa.cc") { + t.Fatalf("skipped conflict must not be reported unclaimed, got %v", result.Annotate.Unclaimed) + } if resolve.Exists(workspacePath) { t.Fatalf("expected resolve state cleared after skip completion") } if files := gitOutput(t, workspacePath, "show", "--name-only", "--format=", "HEAD"); files != "chrome/bbb.cc" { - t.Fatalf("reject junk must stay out of the feature commit, got %q", files) + t.Fatalf("half-applied skip and reject junk must stay out of the feature commit, got %q", files) + } + if status := gitOutput(t, workspacePath, "status", "--porcelain", "--", "chrome/aaa.cc"); status != "M chrome/aaa.cc" { + t.Fatalf("skipped conflict should stay dirty for the user, got %q", status) + } +} + +func TestApplyRefusesDuringConflictResolution(t *testing.T) { + ctx := context.Background() + workspacePath := initGitRepo(t) + writeFile(t, filepath.Join(workspacePath, "chrome", "core.cc"), "base\n") + runGit(t, workspacePath, "add", "chrome/core.cc") + runGit(t, workspacePath, "commit", "-m", "workspace base") + baseCommit := gitOutput(t, workspacePath, "rev-parse", "HEAD") + + repoInfo := newPatchRepo(t, baseCommit) + if err := resolve.Save(workspacePath, &resolve.State{ + Workspace: workspacePath, + RepoRoot: repoInfo.Root, + BaseCommit: baseCommit, + Operations: []resolve.Operation{{ChromiumPath: "chrome/core.cc", PatchRel: "chrome/core.cc", Op: patch.OpModify}}, + }); err != nil { + t.Fatalf("resolve.Save: %v", err) + } + + ws := workspace.Entry{Name: "ws", Path: workspacePath} + if _, err := Apply(ctx, ApplyOptions{Workspace: ws, Repo: repoInfo}); err == nil || !strings.Contains(err.Error(), "conflict resolution is in progress") { + t.Fatalf("expected apply to refuse during pending resolution, got %v", err) + } + if _, err := Sync(ctx, SyncOptions{Workspace: ws, Repo: repoInfo}); err == nil || !strings.Contains(err.Error(), "conflict resolution is in progress") { + t.Fatalf("expected sync to refuse during pending resolution, got %v", err) + } + if !resolve.Exists(workspacePath) { + t.Fatalf("pending resolve state must survive the refused runs") + } +} + +func TestApplyAutoAnnotateSkipsWithoutFeaturesRegistry(t *testing.T) { + ctx := context.Background() + workspacePath := initGitRepo(t) + writeFile(t, filepath.Join(workspacePath, "chrome", "core.cc"), "base\n") + runGit(t, workspacePath, "add", "chrome/core.cc") + runGit(t, workspacePath, "commit", "-m", "workspace base") + baseCommit := gitOutput(t, workspacePath, "rev-parse", "HEAD") + + repoInfo := newPatchRepo(t, baseCommit) + writePatchFromEdit(t, ctx, workspacePath, repoInfo, baseCommit, "chrome/core.cc", "patched\n") + + result, err := Apply(ctx, ApplyOptions{ + Workspace: workspace.Entry{Name: "ws", Path: workspacePath}, + Repo: repoInfo, + AutoAnnotate: true, + }) + if err != nil { + t.Fatalf("Apply must succeed without a features registry: %v", err) + } + if len(result.Applied) != 1 { + t.Fatalf("expected the patch applied, got %v", result.Applied) + } + if result.Annotate != nil || result.AnnotateError != "" { + t.Fatalf("expected annotation skipped quietly, got %+v / %q", result.Annotate, result.AnnotateError) + } +} + +func TestApplyRecordsAnnotateErrorWithoutFailing(t *testing.T) { + ctx := context.Background() + workspacePath := initGitRepo(t) + writeFile(t, filepath.Join(workspacePath, "chrome", "core.cc"), "base\n") + runGit(t, workspacePath, "add", "chrome/core.cc") + runGit(t, workspacePath, "commit", "-m", "workspace base") + baseCommit := gitOutput(t, workspacePath, "rev-parse", "HEAD") + + repoInfo := newPatchRepo(t, baseCommit) + writePatchFromEdit(t, ctx, workspacePath, repoInfo, baseCommit, "chrome/core.cc", "patched\n") + writeFeaturesYAML(t, repoInfo.Root, "features: broken\n") + + result, err := Apply(ctx, ApplyOptions{ + Workspace: workspace.Entry{Name: "ws", Path: workspacePath}, + Repo: repoInfo, + AutoAnnotate: true, + }) + if err != nil { + t.Fatalf("Apply must not fail on an annotate error: %v", err) + } + if len(result.Applied) != 1 { + t.Fatalf("apply outcome must survive the annotate failure, got %v", result.Applied) + } + if result.AnnotateError == "" || result.Annotate != nil { + t.Fatalf("expected recorded annotate error, got %+v / %q", result.Annotate, result.AnnotateError) } + assertFile(t, filepath.Join(workspacePath, "chrome", "core.cc"), "patched\n") } func TestInspectWorkspaceSkipsIgnoredUntrackedFiles(t *testing.T) { diff --git a/packages/browseros/tools/patch/internal/engine/sync.go b/packages/browseros/tools/patch/internal/engine/sync.go index be11403d3a..95ceec16e2 100644 --- a/packages/browseros/tools/patch/internal/engine/sync.go +++ b/packages/browseros/tools/patch/internal/engine/sync.go @@ -34,6 +34,9 @@ type SyncResult struct { } func Sync(ctx context.Context, opts SyncOptions) (*SyncResult, error) { + if err := requireNoPendingResolution(opts.Workspace); err != nil { + return nil, err + } if opts.Remote == "" { opts.Remote = "origin" } From ef1f1e1d0b24fca0e72624b196186019860a16f9 Mon Sep 17 00:00:00 2001 From: Nikhil Sonti Date: Thu, 2 Jul 2026 16:49:06 -0700 Subject: [PATCH 3/4] fix(patches): close second-round auto-annotate review gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - skipped rename conflicts now exclude BOTH the new and old paths from auto-annotate, so a half-applied rename's old-file deletion is never committed into feature history - the pending-resolution guard also refuses on unmerged (UU) index entries — the state a conflicted stash pop leaves — and auto-annotate skips a checkout with a live pending stash, so conflict markers and parked local changes never land in feature commits - resolve.Save writes atomically (temp + rename) so a crash mid-write can't leave a corrupt resolve.json that wedges continue/skip/abort - an annotate failure after a skip now appends the still-uncommitted skipped paths to the error, so following the recovery hint does not sweep them in - annotate consumes one status snapshot via in-memory partition instead of rescanning after every feature commit - skipped reasons (no registry, pending stash) surface in output and --json (annotate_skipped); Annotate reuses the shared guard helper --- packages/browseros/tools/patch/cmd/common.go | 3 + .../tools/patch/internal/engine/annotate.go | 50 ++++----- .../tools/patch/internal/engine/apply.go | 103 +++++++++++++++--- .../patch/internal/engine/engine_test.go | 48 ++++++++ .../tools/patch/internal/engine/sync.go | 2 +- .../tools/patch/internal/resolve/resolve.go | 26 ++++- 6 files changed, 189 insertions(+), 43 deletions(-) diff --git a/packages/browseros/tools/patch/cmd/common.go b/packages/browseros/tools/patch/cmd/common.go index 41f340aee7..f6b54aba04 100644 --- a/packages/browseros/tools/patch/cmd/common.go +++ b/packages/browseros/tools/patch/cmd/common.go @@ -142,6 +142,9 @@ func printAnnotateOutcome(ws workspace.Entry, result *engine.ApplyResult) { fmt.Println() printAnnotateResult(ws, result.Annotate) } + if result.AnnotateSkipped != "" { + fmt.Println(ui.Hint(fmt.Sprintf("Annotation skipped: %s.", result.AnnotateSkipped))) + } if result.AnnotateError != "" { fmt.Println(ui.Warning("Patches applied, but annotate failed")) fmt.Printf(" %s\n", result.AnnotateError) diff --git a/packages/browseros/tools/patch/internal/engine/annotate.go b/packages/browseros/tools/patch/internal/engine/annotate.go index 3d1de3bbbc..8bc965e95f 100644 --- a/packages/browseros/tools/patch/internal/engine/annotate.go +++ b/packages/browseros/tools/patch/internal/engine/annotate.go @@ -8,7 +8,6 @@ import ( "github.com/browseros-ai/BrowserOS/packages/browseros/tools/patch/internal/git" "github.com/browseros-ai/BrowserOS/packages/browseros/tools/patch/internal/patch" "github.com/browseros-ai/BrowserOS/packages/browseros/tools/patch/internal/repo" - "github.com/browseros-ai/BrowserOS/packages/browseros/tools/patch/internal/resolve" "github.com/browseros-ai/BrowserOS/packages/browseros/tools/patch/internal/workspace" "gopkg.in/yaml.v3" ) @@ -58,10 +57,8 @@ type annotateFileSet struct { // It refuses to run mid-conflict-resolution: committing a half-applied tree // would bake reject files and partial patches into feature history. func Annotate(ctx context.Context, opts AnnotateOptions) (*AnnotateResult, error) { - if resolve.Exists(opts.Workspace.Path) { - return nil, fmt.Errorf( - "conflict resolution is in progress for %s; finish it with \"browseros-patch continue\", \"browseros-patch skip\", or \"browseros-patch abort\" before annotating", - opts.Workspace.Name) + if err := requireNoPendingResolution(ctx, opts.Workspace); err != nil { + return nil, err } features, featuresFile, err := LoadFeatures(opts.Repo) if err != nil { @@ -77,15 +74,14 @@ func Annotate(ctx context.Context, opts AnnotateOptions) (*AnnotateResult, error Committed: []AnnotateCommittedFeature{}, Skipped: []AnnotateSkippedFeature{}, } - // One status snapshot serves consecutive features; a full scan is seconds - // on a Chromium checkout. Rescan only after a commit changes what is - // dirty, and once more before the unclaimed report if staging may have - // normalized entries without committing. + // One status snapshot serves the whole run; a full scan is seconds on a + // Chromium checkout. Each feature consumes the entries it matched — + // commitFeatureFiles settles them (committed, or staged clean) — so what + // remains at the end is exactly the unclaimed leftovers. changes, err := annotateChanges(ctx, opts.Workspace.Path, ignore, opts.Exclude) if err != nil { return nil, err } - staged := false for _, feature := range features { result.Processed++ reportProgress(opts.Progress, "Annotating feature %s", feature.Name) @@ -98,7 +94,8 @@ func Annotate(ctx context.Context, opts AnnotateOptions) (*AnnotateResult, error result.FeaturesSkipped++ continue } - files := modifiedFeatureFiles(changes, feature) + matched, rest := partitionFeatureChanges(changes, feature) + files := annotateFileSetFrom(matched) if len(files.report) == 0 { result.Skipped = append(result.Skipped, AnnotateSkippedFeature{ Name: feature.Name, @@ -112,7 +109,7 @@ func Annotate(ctx context.Context, opts AnnotateOptions) (*AnnotateResult, error if err != nil { return nil, fmt.Errorf("commit feature %s: %w", feature.Name, err) } - staged = true + changes = rest if !committed { result.Skipped = append(result.Skipped, AnnotateSkippedFeature{ Name: feature.Name, @@ -129,15 +126,6 @@ func Annotate(ctx context.Context, opts AnnotateOptions) (*AnnotateResult, error Files: committedFiles, }) result.CommitsCreated++ - if changes, err = annotateChanges(ctx, opts.Workspace.Path, ignore, opts.Exclude); err != nil { - return nil, err - } - staged = false - } - if staged { - if changes, err = annotateChanges(ctx, opts.Workspace.Path, ignore, opts.Exclude); err != nil { - return nil, err - } } result.Unclaimed = uniqueReportPaths(changes) return result, nil @@ -222,15 +210,27 @@ func uniqueReportPaths(changes []git.FileChange) []string { return paths } -func modifiedFeatureFiles(changes []git.FileChange, feature FeatureSpec) annotateFileSet { +// partitionFeatureChanges splits the snapshot into the entries a feature +// claims and the rest, so the caller can settle the claimed ones and carry +// the remainder to the next feature without re-scanning git status. +func partitionFeatureChanges(changes []git.FileChange, feature FeatureSpec) ([]git.FileChange, []git.FileChange) { + var matched, rest []git.FileChange + for _, change := range changes { + if featureMatchesChange(feature, change) { + matched = append(matched, change) + continue + } + rest = append(rest, change) + } + return matched, rest +} + +func annotateFileSetFrom(changes []git.FileChange) annotateFileSet { set := annotateFileSet{} reportSeen := map[string]bool{} stageSeen := map[string]bool{} commitSeen := map[string]bool{} for _, change := range changes { - if !featureMatchesChange(feature, change) { - continue - } for _, rel := range changeReportPaths(change) { appendUniquePath(&set.report, reportSeen, rel) } diff --git a/packages/browseros/tools/patch/internal/engine/apply.go b/packages/browseros/tools/patch/internal/engine/apply.go index f868b38a76..4ce29f9698 100644 --- a/packages/browseros/tools/patch/internal/engine/apply.go +++ b/packages/browseros/tools/patch/internal/engine/apply.go @@ -51,10 +51,13 @@ type ApplyResult struct { // apply outcome: the patches landed and the resolve state is already // settled, so the recovery is a standalone annotate, not a retry. AnnotateError string `json:"annotate_error,omitempty"` + // AnnotateSkipped names why auto-annotate did not run (no features + // registry, pending stash) so a missing annotate section is explained. + AnnotateSkipped string `json:"annotate_skipped,omitempty"` } func Apply(ctx context.Context, opts ApplyOptions) (*ApplyResult, error) { - if err := requireNoPendingResolution(opts.Workspace); err != nil { + if err := requireNoPendingResolution(ctx, opts.Workspace); err != nil { return nil, err } repoRev, err := git.HeadRev(ctx, opts.Repo.Root) @@ -93,16 +96,27 @@ func Apply(ctx context.Context, opts ApplyOptions) (*ApplyResult, error) { return result, nil } -// requireNoPendingResolution stops a new run from clobbering a paused -// conflict resolution: the old resolve state would be silently overwritten or -// deleted, stranding continue/skip and losing the run's persisted intent. -func requireNoPendingResolution(ws workspace.Entry) error { - if !resolve.Exists(ws.Path) { - return nil +// requireNoPendingResolution stops a new run from operating on a paused or +// conflicted checkout: a pending resolve state would be silently clobbered +// (stranding continue/skip and losing the run's persisted intent), and +// unmerged index entries mean a stash pop is waiting on the user — committing +// or re-applying over either bakes conflict debris into the tree. +func requireNoPendingResolution(ctx context.Context, ws workspace.Entry) error { + if resolve.Exists(ws.Path) { + return fmt.Errorf( + "conflict resolution is in progress for %s; finish it with \"browseros-patch continue\", \"browseros-patch skip\", or \"browseros-patch abort\" first", + ws.Name) + } + unmerged, err := git.UnmergedFiles(ctx, ws.Path) + if err != nil { + return err } - return fmt.Errorf( - "conflict resolution is in progress for %s; finish it with \"browseros-patch continue\", \"browseros-patch skip\", or \"browseros-patch abort\" first", - ws.Name) + if len(unmerged) > 0 { + return fmt.Errorf( + "%s has unresolved merge conflicts (%s); resolve them before running this command", + ws.Name, strings.Join(unmerged, ", ")) + } + return nil } // finishApply records completion state and, when requested, turns the applied @@ -122,14 +136,35 @@ func finishApply(ctx context.Context, opts ApplyOptions, repoRev string, result } // annotateApplied runs the auto-annotate step of a completed apply. A repo -// without a features registry skips quietly, and a failure is recorded on the -// result instead of returned: the apply itself succeeded and its state is -// already settled, so the caller must still see what was applied. +// without a features registry or a checkout with a live pending stash skips +// with a recorded reason, and a failure is recorded on the result instead of +// returned: the apply itself succeeded and its state is already settled, so +// the caller must still see what was applied. func annotateApplied(ctx context.Context, ws workspace.Entry, repoInfo *repo.Info, exclude []string, result *ApplyResult, progress Progress) { if _, err := FeatureFilePath(repoInfo.Root); err != nil { - reportProgress(progress, "No features registry; skipping annotation") + result.AnnotateSkipped = "no features registry" + return + } + // A recorded stash that still exists means local changes are parked or a + // stash rebase conflicted; either way the tree is not purely patches, so + // auto-committing feature history could sweep user work or conflict + // markers. A standalone annotate stays available once the stash settles. + state, err := workspace.LoadState(ws.Path) + if err != nil { + result.AnnotateError = err.Error() return } + if state.PendingStash != "" { + live, err := git.StashEntryExists(ctx, ws.Path, state.PendingStash) + if err != nil { + result.AnnotateError = err.Error() + return + } + if live { + result.AnnotateSkipped = "pending stash holds local changes" + return + } + } annotateResult, err := Annotate(ctx, AnnotateOptions{ Workspace: ws, Repo: repoInfo, @@ -138,6 +173,13 @@ func annotateApplied(ctx context.Context, ws workspace.Entry, repoInfo *repo.Inf }) if err != nil { result.AnnotateError = err.Error() + if len(exclude) > 0 { + // A standalone annotate would not know about these: warn before + // the user follows the recovery hint and sweeps them in. + result.AnnotateError += fmt.Sprintf( + "; skipped conflicts remain uncommitted (%s) — resolve or reset them before annotating", + strings.Join(exclude, ", ")) + } return } result.Annotate = annotateResult @@ -244,7 +286,7 @@ func finishResolvedRun(ctx context.Context, ws workspace.Entry, repoInfo *repo.I return err } if state.AutoAnnotate { - annotateApplied(ctx, ws, repoInfo, state.Skipped, result, progress) + annotateApplied(ctx, ws, repoInfo, skippedExcludePaths(state), result, progress) } if state.RestorePendingStash { if err := restorePendingStash(ctx, ws.Path, result, progress); err != nil { @@ -254,6 +296,37 @@ func finishResolvedRun(ctx context.Context, ws workspace.Entry, repoInfo *repo.I return nil } +// skippedExcludePaths returns every checkout path a skipped conflict may have +// touched. A skipped rename leaves the old path deleted and the new path +// half-written, so both must be excluded from auto-annotate — otherwise the +// old-path deletion gets committed into a feature as a broken half-rename. +func skippedExcludePaths(state *resolve.State) []string { + if len(state.Skipped) == 0 { + return nil + } + skipped := make(map[string]bool, len(state.Skipped)) + for _, rel := range state.Skipped { + skipped[rel] = true + } + seen := map[string]bool{} + var paths []string + add := func(rel string) { + if rel == "" || seen[rel] { + return + } + seen[rel] = true + paths = append(paths, rel) + } + for _, op := range state.Operations { + if !skipped[op.ChromiumPath] { + continue + } + add(op.ChromiumPath) + add(op.OldPath) + } + return paths +} + // restorePendingStash pops a stash a conflicted sync left behind once the // last operation completes, so the rebase loop ends with local changes back // in the tree. A pop conflict keeps the stash pending and is reported on the diff --git a/packages/browseros/tools/patch/internal/engine/engine_test.go b/packages/browseros/tools/patch/internal/engine/engine_test.go index d2ac2015c8..cb0e4c8ec8 100644 --- a/packages/browseros/tools/patch/internal/engine/engine_test.go +++ b/packages/browseros/tools/patch/internal/engine/engine_test.go @@ -846,6 +846,23 @@ features: } } +func TestSkippedExcludePathsCoversRenameOldPath(t *testing.T) { + state := &resolve.State{ + Skipped: []string{"chrome/new.cc", "content/plain.cc"}, + Operations: []resolve.Operation{ + {ChromiumPath: "chrome/new.cc", OldPath: "chrome/old.cc", Op: patch.OpRename}, + {ChromiumPath: "content/plain.cc", Op: patch.OpModify}, + {ChromiumPath: "chrome/applied.cc", Op: patch.OpModify}, + }, + } + got := skippedExcludePaths(state) + slices.Sort(got) + want := []string{"chrome/new.cc", "chrome/old.cc", "content/plain.cc"} + if !slices.Equal(got, want) { + t.Fatalf("skippedExcludePaths = %v, want %v (rename old path and skipped paths, not applied ops)", got, want) + } +} + func TestApplyRefusesDuringConflictResolution(t *testing.T) { ctx := context.Background() workspacePath := initGitRepo(t) @@ -876,6 +893,37 @@ func TestApplyRefusesDuringConflictResolution(t *testing.T) { } } +func TestApplyRefusesWithUnmergedFiles(t *testing.T) { + ctx := context.Background() + workspacePath := initGitRepo(t) + rel := "chrome/core.cc" + writeFile(t, filepath.Join(workspacePath, rel), "base\n") + runGit(t, workspacePath, "add", rel) + runGit(t, workspacePath, "commit", "-m", "workspace base") + baseCommit := gitOutput(t, workspacePath, "rev-parse", "HEAD") + + // Two branches touching the same line, merged to force an unmerged (UU) + // entry — the state a conflicted stash pop leaves, with no resolve.json. + runGit(t, workspacePath, "checkout", "-b", "other") + writeFile(t, filepath.Join(workspacePath, rel), "other\n") + runGit(t, workspacePath, "commit", "-am", "other change") + runGit(t, workspacePath, "checkout", "-") + writeFile(t, filepath.Join(workspacePath, rel), "mine\n") + runGit(t, workspacePath, "commit", "-am", "my change") + if err := exec.Command("git", "-C", workspacePath, "merge", "other").Run(); err == nil { + t.Fatalf("expected merge to conflict") + } + if status := gitOutput(t, workspacePath, "status", "--porcelain", "--", rel); !strings.HasPrefix(status, "UU") { + t.Fatalf("test setup expected unmerged status, got %q", status) + } + + repoInfo := newPatchRepo(t, baseCommit) + ws := workspace.Entry{Name: "ws", Path: workspacePath} + if _, err := Apply(ctx, ApplyOptions{Workspace: ws, Repo: repoInfo}); err == nil || !strings.Contains(err.Error(), "unresolved merge conflicts") { + t.Fatalf("expected apply to refuse on unmerged files, got %v", err) + } +} + func TestApplyAutoAnnotateSkipsWithoutFeaturesRegistry(t *testing.T) { ctx := context.Background() workspacePath := initGitRepo(t) diff --git a/packages/browseros/tools/patch/internal/engine/sync.go b/packages/browseros/tools/patch/internal/engine/sync.go index 95ceec16e2..33c9bcbce8 100644 --- a/packages/browseros/tools/patch/internal/engine/sync.go +++ b/packages/browseros/tools/patch/internal/engine/sync.go @@ -34,7 +34,7 @@ type SyncResult struct { } func Sync(ctx context.Context, opts SyncOptions) (*SyncResult, error) { - if err := requireNoPendingResolution(opts.Workspace); err != nil { + if err := requireNoPendingResolution(ctx, opts.Workspace); err != nil { return nil, err } if opts.Remote == "" { diff --git a/packages/browseros/tools/patch/internal/resolve/resolve.go b/packages/browseros/tools/patch/internal/resolve/resolve.go index 018c57ad73..c4413b47cc 100644 --- a/packages/browseros/tools/patch/internal/resolve/resolve.go +++ b/packages/browseros/tools/patch/internal/resolve/resolve.go @@ -60,14 +60,36 @@ func Load(workspacePath string) (*State, error) { } func Save(workspacePath string, state *State) error { - if err := os.MkdirAll(workspace.StateDir(workspacePath), 0o755); err != nil { + dir := workspace.StateDir(workspacePath) + if err := os.MkdirAll(dir, 0o755); err != nil { return err } body, err := json.MarshalIndent(state, "", " ") if err != nil { return err } - return os.WriteFile(Path(workspacePath), append(body, '\n'), 0o644) + // Write atomically: a crash mid-write would otherwise leave a truncated + // resolve.json that every recovery command (continue/skip/abort) fails to + // parse, wedging the checkout. + tmp, err := os.CreateTemp(dir, "resolve-*.json.tmp") + if err != nil { + return err + } + tmpName := tmp.Name() + if _, err := tmp.Write(append(body, '\n')); err != nil { + tmp.Close() + _ = os.Remove(tmpName) + return err + } + if err := tmp.Close(); err != nil { + _ = os.Remove(tmpName) + return err + } + if err := os.Rename(tmpName, Path(workspacePath)); err != nil { + _ = os.Remove(tmpName) + return err + } + return nil } func Delete(workspacePath string) error { From 5f9c9bd68566dbb12d78f4e43e580f309bc4612a Mon Sep 17 00:00:00 2001 From: Nikhil Sonti Date: Thu, 2 Jul 2026 17:07:00 -0700 Subject: [PATCH 4/4] fix(patches): guard annotate on conflict markers, not pending-stash presence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-3 review found the pending-stash skip cut both ways: it over-blocked the safe case (a --no-rebase sync parks changes in the stash, leaving the worktree pure patches) while the standalone annotate command stayed unprotected against the unsafe case (a stash rebase leaves conflict markers as plain modified files, not unmerged index entries). Replace it with a precise signal: requireNoPendingResolution — shared by apply, sync, and annotate — now refuses when git's own detector (git diff --check) reports leftover conflict markers. A parked-clean stash has no markers and annotates normally; a stash-rebase conflict is blocked on every path. --- .../tools/patch/internal/engine/apply.go | 48 +++++------- .../patch/internal/engine/engine_test.go | 76 +++++++++++++++++++ .../browseros/tools/patch/internal/git/git.go | 34 +++++++++ 3 files changed, 131 insertions(+), 27 deletions(-) diff --git a/packages/browseros/tools/patch/internal/engine/apply.go b/packages/browseros/tools/patch/internal/engine/apply.go index 4ce29f9698..b53f3f2058 100644 --- a/packages/browseros/tools/patch/internal/engine/apply.go +++ b/packages/browseros/tools/patch/internal/engine/apply.go @@ -98,9 +98,11 @@ func Apply(ctx context.Context, opts ApplyOptions) (*ApplyResult, error) { // requireNoPendingResolution stops a new run from operating on a paused or // conflicted checkout: a pending resolve state would be silently clobbered -// (stranding continue/skip and losing the run's persisted intent), and -// unmerged index entries mean a stash pop is waiting on the user — committing -// or re-applying over either bakes conflict debris into the tree. +// (stranding continue/skip and losing the run's persisted intent), unmerged +// index entries mean a stash pop is waiting on the user, and leftover conflict +// markers mean a stash rebase left work half-merged (git leaves those as plain +// modified files, not unmerged entries). Committing or re-applying over any of +// these bakes conflict debris into the tree. func requireNoPendingResolution(ctx context.Context, ws workspace.Entry) error { if resolve.Exists(ws.Path) { return fmt.Errorf( @@ -116,6 +118,15 @@ func requireNoPendingResolution(ctx context.Context, ws workspace.Entry) error { "%s has unresolved merge conflicts (%s); resolve them before running this command", ws.Name, strings.Join(unmerged, ", ")) } + markers, err := git.ConflictMarkerFiles(ctx, ws.Path) + if err != nil { + return err + } + if len(markers) > 0 { + return fmt.Errorf( + "%s has leftover conflict markers (%s); resolve them (then \"git stash drop\" if a sync left a stash) before running this command", + ws.Name, strings.Join(markers, ", ")) + } return nil } @@ -136,35 +147,18 @@ func finishApply(ctx context.Context, opts ApplyOptions, repoRev string, result } // annotateApplied runs the auto-annotate step of a completed apply. A repo -// without a features registry or a checkout with a live pending stash skips -// with a recorded reason, and a failure is recorded on the result instead of -// returned: the apply itself succeeded and its state is already settled, so -// the caller must still see what was applied. +// without a features registry skips with a recorded reason, and a failure is +// recorded on the result instead of returned: the apply itself succeeded and +// its state is already settled, so the caller must still see what was applied. +// A parked --no-rebase stash needs no special case here — its local changes +// sit in the stash, not the worktree, so the tree is pure patches and annotate +// commits them correctly; the shared guard blocks the unsafe case (conflict +// markers from a stash rebase) inside Annotate. func annotateApplied(ctx context.Context, ws workspace.Entry, repoInfo *repo.Info, exclude []string, result *ApplyResult, progress Progress) { if _, err := FeatureFilePath(repoInfo.Root); err != nil { result.AnnotateSkipped = "no features registry" return } - // A recorded stash that still exists means local changes are parked or a - // stash rebase conflicted; either way the tree is not purely patches, so - // auto-committing feature history could sweep user work or conflict - // markers. A standalone annotate stays available once the stash settles. - state, err := workspace.LoadState(ws.Path) - if err != nil { - result.AnnotateError = err.Error() - return - } - if state.PendingStash != "" { - live, err := git.StashEntryExists(ctx, ws.Path, state.PendingStash) - if err != nil { - result.AnnotateError = err.Error() - return - } - if live { - result.AnnotateSkipped = "pending stash holds local changes" - return - } - } annotateResult, err := Annotate(ctx, AnnotateOptions{ Workspace: ws, Repo: repoInfo, diff --git a/packages/browseros/tools/patch/internal/engine/engine_test.go b/packages/browseros/tools/patch/internal/engine/engine_test.go index cb0e4c8ec8..eaecb8892e 100644 --- a/packages/browseros/tools/patch/internal/engine/engine_test.go +++ b/packages/browseros/tools/patch/internal/engine/engine_test.go @@ -924,6 +924,82 @@ func TestApplyRefusesWithUnmergedFiles(t *testing.T) { } } +func TestAnnotateRefusesOnConflictMarkers(t *testing.T) { + ctx := context.Background() + workspacePath := initGitRepo(t) + rel := "chrome/core.cc" + writeFile(t, filepath.Join(workspacePath, rel), "base\n") + runGit(t, workspacePath, "add", rel) + runGit(t, workspacePath, "commit", "-m", "workspace base") + baseCommit := gitOutput(t, workspacePath, "rev-parse", "HEAD") + + // The plain-modified-with-markers state a stash rebase leaves (no UU entry). + writeFile(t, filepath.Join(workspacePath, rel), "<<<<<<< local\nmine\n=======\ntheirs\n>>>>>>> stashed\n") + + repoInfo := newPatchRepo(t, baseCommit) + writeFeaturesYAML(t, repoInfo.Root, `version: "1.0" +features: + browseros-core: + description: "chore: core" + files: + - chrome/ +`) + ws := workspace.Entry{Name: "ws", Path: workspacePath} + if _, err := Annotate(ctx, AnnotateOptions{Workspace: ws, Repo: repoInfo}); err == nil || !strings.Contains(err.Error(), "leftover conflict markers") { + t.Fatalf("expected annotate to refuse on conflict markers, got %v", err) + } + if _, err := Apply(ctx, ApplyOptions{Workspace: ws, Repo: repoInfo}); err == nil || !strings.Contains(err.Error(), "leftover conflict markers") { + t.Fatalf("expected apply to refuse on conflict markers, got %v", err) + } +} + +func TestApplyAnnotatesWithParkedCleanStash(t *testing.T) { + ctx := context.Background() + workspacePath := initGitRepo(t) + rel := "chrome/core.cc" + writeFile(t, filepath.Join(workspacePath, rel), "base\n") + writeFile(t, filepath.Join(workspacePath, "local.txt"), "local\n") + runGit(t, workspacePath, "add", ".") + runGit(t, workspacePath, "commit", "-m", "workspace base") + baseCommit := gitOutput(t, workspacePath, "rev-parse", "HEAD") + + repoInfo := newPatchRepo(t, baseCommit) + writePatchFromEdit(t, ctx, workspacePath, repoInfo, baseCommit, rel, "patched\n") + writeFeaturesYAML(t, repoInfo.Root, `version: "1.0" +features: + browseros-core: + description: "chore: core" + files: + - chrome/ +`) + + // Simulate a --no-rebase sync that parked local changes: the change is in + // a stash, removed from the worktree, and recorded on workspace state. + writeFile(t, filepath.Join(workspacePath, "local.txt"), "local changed\n") + stashRef, err := git.StashPush(ctx, workspacePath, "parked", true, []string{"local.txt"}) + if err != nil { + t.Fatalf("StashPush: %v", err) + } + if err := workspace.SaveState(workspacePath, &workspace.State{Version: 1, Workspace: workspacePath, BaseCommit: baseCommit, PendingStash: stashRef}); err != nil { + t.Fatalf("SaveState: %v", err) + } + + result, err := Apply(ctx, ApplyOptions{ + Workspace: workspace.Entry{Name: "ws", Path: workspacePath}, + Repo: repoInfo, + AutoAnnotate: true, + }) + if err != nil { + t.Fatalf("Apply: %v", err) + } + if result.AnnotateSkipped != "" { + t.Fatalf("a parked-clean stash must not block annotation, got skip=%q", result.AnnotateSkipped) + } + if result.Annotate == nil || result.Annotate.CommitsCreated != 1 { + t.Fatalf("expected feature commit for the pure-patch tree, got %+v", result.Annotate) + } +} + func TestApplyAutoAnnotateSkipsWithoutFeaturesRegistry(t *testing.T) { ctx := context.Background() workspacePath := initGitRepo(t) diff --git a/packages/browseros/tools/patch/internal/git/git.go b/packages/browseros/tools/patch/internal/git/git.go index 28499b76ef..007807a858 100644 --- a/packages/browseros/tools/patch/internal/git/git.go +++ b/packages/browseros/tools/patch/internal/git/git.go @@ -600,6 +600,40 @@ func UnmergedFiles(ctx context.Context, dir string) ([]string, error) { return splitLines(result.Stdout), nil } +// ConflictMarkerFiles returns tracked files that carry leftover conflict +// markers relative to HEAD — the state a stash-rebase conflict leaves as plain +// modified files (no unmerged index entry). It relies on git's own detector +// ("git diff --check"), filtering to marker lines so trailing-whitespace +// warnings in legitimately patched files are ignored. +func ConflictMarkerFiles(ctx context.Context, dir string) ([]string, error) { + result, err := Run(ctx, dir, nil, "-c", "core.quotepath=false", "diff", "--check", "HEAD") + if err != nil { + return nil, err + } + // --check exits 2 when it reports markers or whitespace; both are expected + // signals, not command failures. A negative code means git never ran. + if result.Code < 0 { + return nil, errors.New(strings.TrimSpace(result.Stderr)) + } + seen := map[string]bool{} + var files []string + for _, line := range splitLines(result.Stdout) { + if !strings.HasSuffix(line, "leftover conflict marker") { + continue + } + path := line + if idx := strings.Index(line, ":"); idx > 0 { + path = line[:idx] + } + if path == "" || seen[path] { + continue + } + seen[path] = true + files = append(files, path) + } + return files, nil +} + // StashRebase replays a stash on top of the current working tree with a // per-file 3-way merge (stash parent as base). Unlike `git stash pop`, it // works when the files were modified after the stash was taken — exactly the