diff --git a/.changes/unreleased/Added-20260602-092301.yaml b/.changes/unreleased/Added-20260602-092301.yaml new file mode 100644 index 00000000..c3767fab --- /dev/null +++ b/.changes/unreleased/Added-20260602-092301.yaml @@ -0,0 +1,3 @@ +kind: Added +body: 'integration rebuild: Add --no-rerere, --reset-resolution-file, and --from-scratch flags to clear cached resolution state, and log resolver/regenerator/rerere invocations so silent replays of stale resolutions are visible.' +time: 2026-06-02T09:23:01.662647-04:00 diff --git a/.changes/unreleased/Changed-20260602-101431.yaml b/.changes/unreleased/Changed-20260602-101431.yaml new file mode 100644 index 00000000..c4501641 --- /dev/null +++ b/.changes/unreleased/Changed-20260602-101431.yaml @@ -0,0 +1,3 @@ +kind: Changed +body: 'integration rebuild: Halt with a resolver-failure error when the configured auto-resolver produces corrupt or unusable output (parse error, non-zero exit, iteration cap). Previously the rebuild fell through to accept-incoming, which routinely dropped integration-side declarations.' +time: 2026-06-02T10:14:31.132085-04:00 diff --git a/.gitignore b/.gitignore index cbe3e72b..3f686058 100644 --- a/.gitignore +++ b/.gitignore @@ -8,3 +8,16 @@ cover.html # Test configuration (contains environment-specific values) internal/forge/forgetest/testconfig.yaml + +# Transient state from 'gs integration rebuild'. Held only for the +# life of the rebuild and removed on exit; should never be committed. +# Was previously swept up by AmendCommitAll's 'git add -A' and baked +# into the integration branch's merge commit on every rebuild. +.spice_rebuild.lock + +# Per-repo resolver state for 'gs integration rebuild'. Carries the +# current_merge pointer (rewritten before each resolver call) and any +# Q&A history the user has answered. The resolver-recipe docs already +# instruct users to gitignore it; same AmendCommitAll path was also +# baking it into integration's merge commits. +.integration_resolution.json diff --git a/doc/includes/cli-reference.md b/doc/includes/cli-reference.md index a501b3c1..9baec81d 100644 --- a/doc/includes/cli-reference.md +++ b/doc/includes/cli-reference.md @@ -317,9 +317,11 @@ Before merging, the stack is checked for branches whose base PR was already merged on the forge. Use --no-branch-check to skip this validation. -Before each merge, waits for CI checks to pass. -Use --build-timeout to configure the maximum wait -before failing if checks are not ready. +Before each merge, waits for merge readiness: +the forge must observe the pushed head +and report that the CR is ready to merge. +Use --ready-timeout to configure the maximum wait +before failing if merge readiness is not reached. By default, a branch failure skips that branch's upstack descendants, but independent sibling branches continue. @@ -328,12 +330,12 @@ Use --fail-fast to stop the queue after the first branch failure. **Flags** * `--method=METHOD` ([:material-wrench:{ .middle title="spice.merge.method" }](/cli/config.md#spicemergemethod)): Preferred merge method. One of 'merge', 'squash', and 'rebase'. -* `--build-timeout=30m` ([:material-wrench:{ .middle title="spice.merge.buildTimeout" }](/cli/config.md#spicemergebuildtimeout)): Max time to wait for CI checks before each merge. 0 means check once. +* `--ready-timeout=30m` ([:material-wrench:{ .middle title="spice.merge.readyTimeout" }](/cli/config.md#spicemergereadytimeout)): Max time to wait for merge readiness before each merge. 0 means check once. * `--no-branch-check`: Skip stale base validation before merging. * `--fail-fast`: Stop the merge queue after the first branch failure. * `--branch=NAME`: Branch whose stack to merge -**Configuration**: [spice.merge.buildTimeout](/cli/config.md#spicemergebuildtimeout), [spice.merge.method](/cli/config.md#spicemergemethod) +**Configuration**: [spice.merge.method](/cli/config.md#spicemergemethod), [spice.merge.readyTimeout](/cli/config.md#spicemergereadytimeout) ### git-spice stack restack {#gs-stack-restack} @@ -631,7 +633,7 @@ This command acts as a local merge queue: it merges one Change Request, waits for that merge to finish, restacks and updates the next Change Request, -waits for its CI checks to pass, +waits for merge readiness on the updated Change Request, and then repeats the process. For a stack like this: @@ -649,13 +651,15 @@ Before merging, the downstack is checked for branches whose base PR was already merged on the forge. Use --no-branch-check to skip this validation. -Before each merge, waits for CI checks to pass. -Use --build-timeout to configure the maximum wait +Before each merge, waits for merge readiness: +the forge must observe the pushed head +and report that the CR is ready to merge. +Use --ready-timeout to configure the maximum wait (default: 30m, 0 means fail immediately if not ready). Between merges, the command waits for each merge to complete, restacks and updates the next PR, -waits for CI checks on the updated PR, +waits for merge readiness on the updated PR, and syncs merged branch cleanup. Use --no-wait for single branch merging @@ -665,12 +669,12 @@ when you don't want to wait for the merge to propagate. **Flags** * `--method=METHOD` ([:material-wrench:{ .middle title="spice.merge.method" }](/cli/config.md#spicemergemethod)): Preferred merge method. One of 'merge', 'squash', and 'rebase'. -* `--build-timeout=30m` ([:material-wrench:{ .middle title="spice.merge.buildTimeout" }](/cli/config.md#spicemergebuildtimeout)): Max time to wait for CI checks before each merge. 0 means check once. +* `--ready-timeout=30m` ([:material-wrench:{ .middle title="spice.merge.readyTimeout" }](/cli/config.md#spicemergereadytimeout)): Max time to wait for merge readiness before each merge. 0 means check once. * `--no-wait`: Skip polling for a single branch merge to propagate. * `--no-branch-check`: Skip stale base validation before merging. * `--branch=NAME`: Branch to start merging from -**Configuration**: [spice.merge.buildTimeout](/cli/config.md#spicemergebuildtimeout), [spice.merge.method](/cli/config.md#spicemergemethod) +**Configuration**: [spice.merge.method](/cli/config.md#spicemergemethod), [spice.merge.readyTimeout](/cli/config.md#spicemergereadytimeout) ### git-spice downstack edit {#gs-downstack-edit} @@ -1140,16 +1144,18 @@ Use --branch to merge a different branch. The branch must be based directly on trunk. To merge a stacked branch, use 'gs downstack merge'. -Before merging, waits for CI checks to pass. -Use --build-timeout to configure the maximum wait. +Before merging, waits for merge readiness: +the forge must observe the pushed head +and report that the CR is ready to merge. +Use --ready-timeout to configure the maximum wait. **Flags** * `--method=METHOD` ([:material-wrench:{ .middle title="spice.merge.method" }](/cli/config.md#spicemergemethod)): Preferred merge method. One of 'merge', 'squash', and 'rebase'. -* `--build-timeout=30m` ([:material-wrench:{ .middle title="spice.merge.buildTimeout" }](/cli/config.md#spicemergebuildtimeout)): Max time to wait for CI checks before each merge. 0 means check once. +* `--ready-timeout=30m` ([:material-wrench:{ .middle title="spice.merge.readyTimeout" }](/cli/config.md#spicemergereadytimeout)): Max time to wait for merge readiness before each merge. 0 means check once. * `--branch=NAME`: Branch to merge -**Configuration**: [spice.merge.buildTimeout](/cli/config.md#spicemergebuildtimeout), [spice.merge.method](/cli/config.md#spicemergemethod) +**Configuration**: [spice.merge.method](/cli/config.md#spicemergemethod), [spice.merge.readyTimeout](/cli/config.md#spicemergereadytimeout) ### git-spice branch submit {#gs-branch-submit} @@ -1454,17 +1460,43 @@ configured resolver script is invoked to attempt automatic resolution before surfacing conflicts. See the recipe for details on the JSON protocol the script must implement. -Any conflicts that survive the merge drivers and the resolver -are auto-resolved by taking the incoming tip's version. Pass ---no-accept-incoming (or set spice.integration.acceptIncoming -=false) to disable that final fallback and surface conflicts -for manual resolution instead. +If the resolver produces corrupt or unusable output (script +exit failure, malformed JSON, missing markers) the rebuild +halts rather than falling through to accept-incoming. The +usual cause is a prompt, model, or script issue that needs +to be fixed, not glossed over by silently picking 'theirs'. + +Conflicts that survive the merge drivers and a successful +resolver run are auto-resolved by taking the incoming tip's +version. Pass --no-accept-incoming (or set +spice.integration.acceptIncoming=false) to disable that +final fallback and surface conflicts for manual resolution +instead. + +If a bad resolution was silently cached (in rerere, in the +resolution file, or in pending rebuild state) and is being +replayed on every rebuild, use --reset-rerere-cache to wipe +stale postimages (while still recording fresh ones), +--reset-resolution-file to wipe the Q&A history, +--reset-pending to drop any stale resume point, or +--from-scratch for all three. + +--no-rerere is a stronger diagnostic mode: it disables +rerere recording too, so the rebuild produces nothing +cached for next time. Use it only when you suspect rerere +itself is misbehaving — otherwise --reset-rerere-cache is +what you want. **Flags** * `--push`: Also push the integration branch after rebuilding * `--[no-]auto-resolve` ([:material-wrench:{ .middle title="spice.integration.autoResolve" }](/cli/config.md#spiceintegrationautoresolve)): Auto-resolve merge conflicts using the configured resolver script * `--[no-]accept-incoming` ([:material-wrench:{ .middle title="spice.integration.acceptIncoming" }](/cli/config.md#spiceintegrationacceptincoming)): Final-stage fallback: take the incoming tip's version for any remaining conflicts so the rebuild completes without manual intervention +* `--no-rerere`: Disable rerere entirely (no replay AND no recording) for this rebuild. Diagnostic mode — prefer --reset-rerere-cache when you want fresh resolutions cached for next time. +* `--reset-rerere-cache`: Wipe the rerere cache (.git/rr-cache) before starting so stale cached resolutions are not replayed. Rerere stays enabled so the rebuild's fresh resolutions are recorded. +* `--reset-resolution-file`: Delete the resolution file (.integration_resolution.json) before starting so stale Q&A history is not carried into this rebuild. +* `--reset-pending`: Clear any pending rebuild state before starting. Use when a prior halted rebuild left stale state that should not be resumed. +* `--from-scratch`: Shorthand: implies --reset-rerere-cache, --reset-resolution-file, and --reset-pending. Use after a bad rebuild left stale state in any cache; rerere stays enabled so good resolutions get recorded. **Configuration**: [spice.integration.acceptIncoming](/cli/config.md#spiceintegrationacceptincoming), [spice.integration.autoResolve](/cli/config.md#spiceintegrationautoresolve) diff --git a/doc/src/guide/integration.md b/doc/src/guide/integration.md index 3e154db3..d271bea3 100644 --- a/doc/src/guide/integration.md +++ b/doc/src/guide/integration.md @@ -185,6 +185,13 @@ Entries are pruned automatically when their branches are untracked ($$gs branch untrack$$), deleted ($$gs branch delete$$), or removed by $$gs repo sync$$ after the underlying CR merges. +| Response | Behavior | +|-----------------------------------------------------|---------------------------------------------------------------------------------------| +| Empty (or `assumptions` only) | Assumptions logged at info level. Files staged, merge committed. | +| `questions` populated | Each question is asked interactively. Answers append to the resolution file. Resolver is re-invoked. | +| `unresolved_files` populated, no `questions` | Resolver structurally surrendered. The conflict surfaces for manual resolution; `--accept-incoming` is bypassed (the AI itself flagged the file as needing human judgement). | +| Non-zero exit code or invalid JSON | **Rebuild halts** with a resolver-failure error; pending state is saved so you can resume after fixing the underlying problem. `--accept-incoming` is also bypassed, since picking "theirs" wholesale would routinely drop integration-side API surface (methods, fields, getters that the tip branch never had). | + ### Example: Claude Code resolver The resolver is configured as a user-level preference — the config @@ -199,7 +206,11 @@ and turn auto-resolve on by default: ```bash git config --global spice.integration.resolver "$(cat <<'GITCONFIG' #!/bin/sh -exec claude --print --max-turns 30 <<'PROMPT' +RAW=$(mktemp -t spice-resolver-raw.XXXXXX) +echo "spice resolver: raw Claude output -> $RAW" >&2 +claude --print --max-turns 30 --output-format text 2>/dev/null <<'PROMPT' \ + | tee "$RAW" \ + | sed -n '//,/<\/resolution>/{//d;/<\/resolution>/d;p;}' You are resolving merge conflicts on a throwaway integration branch. CONTEXT @@ -220,9 +231,27 @@ WHAT TO DO - Do NOT run 'git add' or 'git commit'. After you exit, git-spice stages every originally-conflicted path and commits the merge. -OUTPUT — emit exactly one JSON document on stdout, then exit: - +DEFAULT TO ADDITIVE MERGES. +When each side adds an independent top-level declaration at the +conflict boundary — a method on a struct, an exported package +function, a struct field, an interface method, a config getter, a +type definition, an import, a struct literal field — that is almost +never a real conflict. Keep BOTH additions, in either order. +Dropping one side's method or getter to "resolve" the conflict +silently breaks the build of any caller that depends on it. Only +collapse two declarations into one when they have the same name +AND the same signature AND one body is strictly a superset of the +other; otherwise keep both. When in doubt, keep both and add an +assumption — it is much easier for the user to delete a stray +method than to rediscover one that was silently dropped. + +OUTPUT — emit exactly one JSON document between and + tags. Only content between these tags is parsed; any +preface or trailing prose is dropped. Exit after emitting. + + {"assumptions": [...], "questions": [...], "unresolved_files": [...]} + - All three keys are optional. Empty (or assumptions-only) means "everything resolved cleanly"; git-spice will stage and commit. @@ -237,6 +266,16 @@ GITCONFIG git config --global spice.integration.autoResolve true ``` +The `...` marker pattern (paired with the +`sed -n` extraction in the script) mirrors the +[$$spice.messageGenerator$$](../cli/config.md#spicemessagegenerator) +recipe. It is more robust than a strict-JSON contract: Claude +routinely emits a leading prose summary even when told not to, and +the marker lets us discard it without changing the resolver's JSON +parser. If the model omits the markers entirely, sed produces no +output — git-spice then logs an "Auto-resolve failed" warning and +falls through to whichever fallback you have configured. + For Claude Code to run unattended, pre-approve the tools it needs in `~/.claude/settings.json`. The schema is a `permissions` object with an `allow` array of tool patterns diff --git a/integration_rebuild.go b/integration_rebuild.go index fb35f5e7..fc957b4d 100644 --- a/integration_rebuild.go +++ b/integration_rebuild.go @@ -12,9 +12,14 @@ import ( ) type integrationRebuildCmd struct { - Push bool `name:"push" help:"Also push the integration branch after rebuilding"` - AutoResolve bool `name:"auto-resolve" negatable:"" config:"integration.autoResolve" help:"Auto-resolve merge conflicts using the configured resolver script"` - AcceptIncoming bool `name:"accept-incoming" negatable:"" default:"true" config:"integration.acceptIncoming" help:"Final-stage fallback: take the incoming tip's version for any remaining conflicts so the rebuild completes without manual intervention"` + Push bool `name:"push" help:"Also push the integration branch after rebuilding"` + AutoResolve bool `name:"auto-resolve" negatable:"" config:"integration.autoResolve" help:"Auto-resolve merge conflicts using the configured resolver script"` + AcceptIncoming bool `name:"accept-incoming" negatable:"" default:"true" config:"integration.acceptIncoming" help:"Final-stage fallback: take the incoming tip's version for any remaining conflicts so the rebuild completes without manual intervention"` + NoRerere bool `name:"no-rerere" help:"Disable rerere entirely (no replay AND no recording) for this rebuild. Diagnostic mode — prefer --reset-rerere-cache when you want fresh resolutions cached for next time."` + ResetRerereCache bool `name:"reset-rerere-cache" help:"Wipe the rerere cache (.git/rr-cache) before starting so stale cached resolutions are not replayed. Rerere stays enabled so the rebuild's fresh resolutions are recorded."` + ResetResolutionFile bool `name:"reset-resolution-file" help:"Delete the resolution file (.integration_resolution.json) before starting so stale Q&A history is not carried into this rebuild."` + ResetPending bool `name:"reset-pending" help:"Clear any pending rebuild state before starting. Use when a prior halted rebuild left stale state that should not be resumed."` + FromScratch bool `name:"from-scratch" help:"Shorthand: implies --reset-rerere-cache, --reset-resolution-file, and --reset-pending. Use after a bad rebuild left stale state in any cache; rerere stays enabled so good resolutions get recorded."` } func (*integrationRebuildCmd) Help() string { @@ -33,11 +38,32 @@ func (*integrationRebuildCmd) Help() string { resolution before surfacing conflicts. See the recipe for details on the JSON protocol the script must implement. - Any conflicts that survive the merge drivers and the resolver - are auto-resolved by taking the incoming tip's version. Pass - --no-accept-incoming (or set spice.integration.acceptIncoming - =false) to disable that final fallback and surface conflicts - for manual resolution instead. + If the resolver produces corrupt or unusable output (script + exit failure, malformed JSON, missing markers) the rebuild + halts rather than falling through to accept-incoming. The + usual cause is a prompt, model, or script issue that needs + to be fixed, not glossed over by silently picking 'theirs'. + + Conflicts that survive the merge drivers and a successful + resolver run are auto-resolved by taking the incoming tip's + version. Pass --no-accept-incoming (or set + spice.integration.acceptIncoming=false) to disable that + final fallback and surface conflicts for manual resolution + instead. + + If a bad resolution was silently cached (in rerere, in the + resolution file, or in pending rebuild state) and is being + replayed on every rebuild, use --reset-rerere-cache to wipe + stale postimages (while still recording fresh ones), + --reset-resolution-file to wipe the Q&A history, + --reset-pending to drop any stale resume point, or + --from-scratch for all three. + + --no-rerere is a stronger diagnostic mode: it disables + rerere recording too, so the rebuild produces nothing + cached for next time. Use it only when you suspect rerere + itself is misbehaving — otherwise --reset-rerere-cache is + what you want. `) } @@ -46,11 +72,44 @@ func (cmd *integrationRebuildCmd) Run( log *silog.Logger, handler IntegrationHandler, ) error { + resetResolution := cmd.ResetResolutionFile || cmd.FromScratch + resetPending := cmd.ResetPending || cmd.FromScratch + resetRerere := cmd.ResetRerereCache || cmd.FromScratch res, err := handler.Rebuild(ctx, &integration.RebuildOptions{ - AutoResolve: &cmd.AutoResolve, - AcceptIncoming: &cmd.AcceptIncoming, + AutoResolve: &cmd.AutoResolve, + AcceptIncoming: &cmd.AcceptIncoming, + NoRerere: cmd.NoRerere, + ResetResolutionFile: resetResolution, + ResetPending: resetPending, + ResetRerereCache: resetRerere, }) if err != nil { + resolverFail := new(integration.ResolverFailedError) + if errors.As(err, &resolverFail) { + var b strings.Builder + b.WriteString("Resolver failed for tip ") + b.WriteString(resolverFail.Tip) + b.WriteString(":\n ") + b.WriteString(resolverFail.Cause.Error()) + b.WriteString("\nConflicted paths (left in worktree):\n") + for _, p := range resolverFail.Paths { + b.WriteString(" - ") + b.WriteString(p) + b.WriteString("\n") + } + b.WriteString( + "Halting on resolver failure to avoid silently dropping " + + "declarations from the integration side. Investigate the " + + "resolver script (spice.integration.resolver) — a corrupt " + + "response usually means the prompt or model is wrong.\n") + b.WriteString("To proceed manually, resolve the conflicts, then run:\n") + b.WriteString(" git merge --continue\n") + b.WriteString("Then resume the rebuild with:\n") + b.WriteString(" " + cli.Name() + " integration rebuild") + log.Error(b.String()) + return err + } + conflict := new(integration.ConflictError) if errors.As(err, &conflict) { var b strings.Builder @@ -73,6 +132,14 @@ func (cmd *integrationRebuildCmd) Run( } log.Infof("Integration branch %q rebuilt with %d tip(s).", res.Name, len(res.TipHashes)) + if res.RegeneratorError != nil { + log.Errorf( + "Regenerator failed during rebuild: %v. The integration "+ + "branch's source is correct but its generated files "+ + "may be stale; run your project's generate target "+ + "before relying on the build.", + res.RegeneratorError) + } if cmd.Push { if err := handler.Submit(ctx); err != nil { diff --git a/internal/git/merge_wt.go b/internal/git/merge_wt.go index cd84349a..2404d177 100644 --- a/internal/git/merge_wt.go +++ b/internal/git/merge_wt.go @@ -23,9 +23,18 @@ type MergeOptions struct { // If empty, Git's default message is used. Message string - // EnableRerere enables rerere.enabled and rerere.autoupdate - // for this merge invocation only. - // User git config is not modified. + // EnableRerere forces rerere's enabled state for this merge + // invocation only. User git config is not modified. + // + // When true: rerere.enabled and rerere.autoupdate are passed as + // -c overrides so rerere replays cached resolutions and + // auto-stages them after replay. + // + // When false: rerere.enabled=false is passed as a -c override. + // This is a force-off, not a "leave alone" — otherwise a user- + // level rerere.enabled=true would keep rerere replaying cached + // resolutions even when the caller explicitly asked for it off + // (e.g., 'gs integration rebuild --no-rerere'). EnableRerere bool // LeaveConflict, when true, leaves a conflicting merge in the @@ -38,6 +47,15 @@ type MergeOptions struct { // by any merge drivers git invokes. Useful for passing per-merge // state to a custom driver (e.g., a log file path). Env []string + + // OnRerereReplay, if non-nil, is called once for each path whose + // conflict was silently resolved from the rr-cache during this + // merge. The path is taken from git's stderr line + // "Resolved 'PATH' using previous resolution." Useful for + // surfacing that an otherwise-clean merge actually replayed a + // cached resolution — silent replays of stale entries are a + // known footgun. + OnRerereReplay func(path string) } // MergeConflictError indicates that a [Worktree.Merge] could not be @@ -85,16 +103,29 @@ func (w *Worktree) Merge(ctx context.Context, opts MergeOptions) error { mergeArgs = append(mergeArgs, opts.Refs...) cmd := w.gitCmd(ctx, "merge", mergeArgs[1:]...) + // EnableRerere is a force, not a hint. When true, override user + // config to enable rerere AND autoupdate. When false, override to + // disable — otherwise a user-level rerere.enabled=true would keep + // rerere replaying cached resolutions even when the caller + // explicitly asked for it off. + var rerereCfg []string if opts.EnableRerere { - prefix := []string{ + rerereCfg = []string{ "-c", "rerere.enabled=true", "-c", "rerere.autoupdate=true", } - cmd = cmd.WithArgs(append(prefix, cmd.Args()...)...) + } else { + rerereCfg = []string{ + "-c", "rerere.enabled=false", + } } + cmd = cmd.WithArgs(append(rerereCfg, cmd.Args()...)...) if len(opts.Env) > 0 { cmd = cmd.AppendEnv(opts.Env...) } + if opts.OnRerereReplay != nil { + cmd.cmd.TeeStderr(&rerereReplayObserver{cb: opts.OnRerereReplay}) + } err := w.runGitWithIndexLockRetry(ctx, func() *gitCmd { return cmd }) if err == nil { diff --git a/internal/git/rerere_observer.go b/internal/git/rerere_observer.go new file mode 100644 index 00000000..325c041a --- /dev/null +++ b/internal/git/rerere_observer.go @@ -0,0 +1,58 @@ +package git + +import "bytes" + +// rerereReplayObserver scans a git stderr byte stream for the rerere +// replay line — git prints "Resolved 'PATH' using previous +// resolution." when rerere.autoupdate is false, or "Staged 'PATH' +// using previous resolution." when autoupdate is true (the merge +// is auto-staged after replay). gs always sets autoupdate=true when +// rerere is enabled, so "Staged" is the dominant form, but we match +// both for robustness across configurations. +// +// It is intended for use with [gitCmd.TeeStderr]. Each matching line +// invokes cb with the captured PATH. Non-matching output is ignored. +// The observer accumulates bytes across Write calls so it correctly +// handles arbitrary chunking. +type rerereReplayObserver struct { + cb func(path string) + buf []byte +} + +var ( + _rerereReplayPrefixResolved = []byte("Resolved '") + _rerereReplayPrefixStaged = []byte("Staged '") + _rerereReplaySuffix = []byte("' using previous resolution.") +) + +func (o *rerereReplayObserver) Write(p []byte) (int, error) { + o.buf = append(o.buf, p...) + for { + idx := bytes.IndexByte(o.buf, '\n') + if idx < 0 { + break + } + line := o.buf[:idx] + o.buf = o.buf[idx+1:] + if path, ok := parseRerereReplay(line); ok { + o.cb(path) + } + } + return len(p), nil +} + +func parseRerereReplay(line []byte) (string, bool) { + var rest []byte + switch { + case bytes.HasPrefix(line, _rerereReplayPrefixResolved): + rest = line[len(_rerereReplayPrefixResolved):] + case bytes.HasPrefix(line, _rerereReplayPrefixStaged): + rest = line[len(_rerereReplayPrefixStaged):] + default: + return "", false + } + if !bytes.HasSuffix(rest, _rerereReplaySuffix) { + return "", false + } + return string(rest[:len(rest)-len(_rerereReplaySuffix)]), true +} diff --git a/internal/git/rerere_observer_test.go b/internal/git/rerere_observer_test.go new file mode 100644 index 00000000..c2342d95 --- /dev/null +++ b/internal/git/rerere_observer_test.go @@ -0,0 +1,66 @@ +package git + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestParseRerereReplay(t *testing.T) { + tests := []struct { + name string + line string + wantPath string + wantOK bool + }{ + { + name: "ResolvedForm", + line: "Resolved 'foo/bar.go' using previous resolution.", + wantPath: "foo/bar.go", + wantOK: true, + }, + { + name: "StagedForm", + line: "Staged 'foo/bar.go' using previous resolution.", + wantPath: "foo/bar.go", + wantOK: true, + }, + { + name: "OtherLine", + line: "Auto-merging foo/bar.go", + }, + { + name: "PartialMatch", + line: "Staged 'foo/bar.go'", + }, + { + name: "Empty", + line: "", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + path, ok := parseRerereReplay([]byte(tt.line)) + assert.Equal(t, tt.wantOK, ok) + assert.Equal(t, tt.wantPath, path) + }) + } +} + +func TestRerereReplayObserver_ChunkedWrites(t *testing.T) { + var got []string + o := &rerereReplayObserver{cb: func(path string) { + got = append(got, path) + }} + // Split across writes mid-line and mid-prefix. + chunks := []string{ + "Auto-merging f.txt\nStag", + "ed 'f.txt' using previous resolution.\nCONFLICT\n", + "Resolved 'g.txt' using previous resolution.\n", + } + for _, c := range chunks { + _, err := o.Write([]byte(c)) + assert.NoError(t, err) + } + assert.Equal(t, []string{"f.txt", "g.txt"}, got) +} diff --git a/internal/handler/integration/handler.go b/internal/handler/integration/handler.go index cb927e8b..1d1df259 100644 --- a/internal/handler/integration/handler.go +++ b/internal/handler/integration/handler.go @@ -10,8 +10,10 @@ import ( "context" "errors" "fmt" + "io/fs" "iter" "os" + "path/filepath" "slices" "strings" @@ -30,6 +32,7 @@ type GitRepository interface { PeelToCommit(ctx context.Context, ref string) (git.Hash, error) ListRemoteRefs(ctx context.Context, remote string, opts *git.ListRemoteRefsOptions) iter.Seq2[git.RemoteRef, error] Worktrees(ctx context.Context) iter.Seq2[*git.WorktreeListItem, error] + GitDir() string } var _ GitRepository = (*git.Repository)(nil) @@ -346,6 +349,15 @@ type RebuildResult struct { // TipHashes holds the hash of each tip used in the rebuild. TipHashes []git.Hash + + // RegeneratorError is the error from the post-merge regenerator, + // if one ran and failed. The rebuild itself succeeded (all tip + // merges committed) and the partial regenerator output, if any, + // was folded into the final merge commit — but the generated + // files in the worktree may no longer match the merged source. + // The CLI surfaces this so "rebuilt with N tips" does not paper + // over a regen failure that leaves the integration build broken. + RegeneratorError error } // ConflictError indicates that a rebuild was interrupted by a merge @@ -363,6 +375,47 @@ func (e *ConflictError) Error() string { return fmt.Sprintf("merge of tip %q conflicted in %d file(s)", e.Tip, len(e.Paths)) } +// ResolverFailedError indicates that the configured auto-resolver was +// invoked but produced corrupt or unusable output (e.g., script exit +// failure, malformed JSON, missing required fields). The merge is +// left in the worktree for the user to resolve manually, and pending +// rebuild state is saved so the rebuild can be resumed. +// +// Halting here — rather than falling through to accept-incoming — is +// deliberate. Modern LLMs reliably produce conforming output when +// prompted correctly; a corrupt response is a signal that the prompt, +// the model, or the resolver script itself needs attention. Silently +// accepting "theirs" instead would routinely drop integration-side +// API surface (methods, getters, fields) and bake the loss into a +// recorded rerere entry. +type ResolverFailedError struct { + // Tip is the name of the tip whose merge invoked the resolver. + Tip string + + // Paths are the files that were passed to the resolver. + Paths []string + + // Cause is the underlying error returned by the resolver. + Cause error +} + +func (e *ResolverFailedError) Error() string { + return fmt.Sprintf( + "resolver failed for tip %q (%d conflicted file(s)): %v", + e.Tip, len(e.Paths), e.Cause) +} + +func (e *ResolverFailedError) Unwrap() error { return e.Cause } + +// errResolverUnresolved is the sentinel returned by autoResolveLoop +// when the resolver responded with a valid structured "give up" — +// non-empty unresolved_files and no questions to ask. It is wrapped +// (via fmt.Errorf %w) so the caller can distinguish a structural +// surrender from a corrupt-output failure: the former drops to +// manual conflict resolution, the latter halts with a +// [*ResolverFailedError]. +var errResolverUnresolved = errors.New("resolver reported unresolved files with no questions") + // RebuildOptions allows callers to override per-invocation behavior. type RebuildOptions struct { // AutoResolve, if non-nil, overrides [Handler.DefaultAutoResolve] @@ -375,6 +428,29 @@ type RebuildOptions struct { // conflicts that remain after the resolver are auto-resolved by // taking the incoming tip's version. AcceptIncoming *bool + + // NoRerere, when true, disables rerere replay and recording for + // this invocation. Use when a previous rebuild may have cached + // a bad resolution that should not be replayed. + NoRerere bool + + // ResetResolutionFile, when true, deletes the resolution file + // (.integration_resolution.json) before starting the rebuild so + // stale Q&A history is not carried into a fresh run. + ResetResolutionFile bool + + // ResetPending, when true, clears any pending integration + // rebuild state before starting so the rebuild starts from + // trunk regardless of where a prior halted rebuild left off. + ResetPending bool + + // ResetRerereCache, when true, deletes the rerere cache + // directory before starting the rebuild. Distinct from + // NoRerere: rerere stays enabled during the rebuild so the + // fresh resolutions get recorded — the wipe just clears any + // bad cached postimages from previous runs that would + // otherwise be replayed silently. + ResetRerereCache bool } // Rebuild regenerates the integration branch by sequentially merging @@ -392,10 +468,43 @@ type RebuildOptions struct { // or [Handler.DefaultAutoResolve]), Rebuild attempts to resolve // conflicts automatically before surfacing them. // +// A repo-scoped file lock at .spice_rebuild.lock ensures that two +// concurrent rebuild invocations (deliberate or otherwise — most +// often a user runs gs intrb in two shells before the first +// completes) cannot race on the worktree, the pending state, or +// the regen log. The second invocation fails fast with a clear +// message instead of corrupting state by interleaving with the +// first. +// // opts may be nil to accept defaults. func (h *Handler) Rebuild( ctx context.Context, opts *RebuildOptions, ) (*RebuildResult, error) { + release, err := acquireRebuildLock(h.RepoRoot) + if err != nil { + return nil, err + } + defer release() + + if opts != nil && opts.ResetResolutionFile { + if err := h.removeResolutionFile(); err != nil { + return nil, fmt.Errorf("reset resolution file: %w", err) + } + } + + if opts != nil && opts.ResetPending { + if err := h.Store.ClearPendingIntegrationRebuild(ctx); err != nil && + !errors.Is(err, state.ErrNotExist) { + return nil, fmt.Errorf("reset pending rebuild: %w", err) + } + } + + if opts != nil && opts.ResetRerereCache { + if err := h.resetRerereCache(); err != nil { + return nil, fmt.Errorf("reset rerere cache: %w", err) + } + } + info, err := h.loadConfigured(ctx) if err != nil { return nil, err @@ -496,6 +605,53 @@ func (h *Handler) ensureWorktreeSafe( return nil } +// rebuildLockFileName is the well-known name of the rebuild lock +// file at the repository root. +const rebuildLockFileName = ".spice_rebuild.lock" + +// acquireRebuildLock takes an exclusive on-disk lock at +// .spice_rebuild.lock relative to repoRoot. Returns a release +// function that the caller must defer. +// +// If the lock file already exists, returns an error naming the lock +// path and the PID recorded in the file (if any) so the user can +// either wait for the other rebuild to finish or remove the stale +// lock by hand. The lock is not crash-safe by design: it is better +// to require a manual cleanup after a hang than to risk two +// concurrent rebuilds clobbering each other. +func acquireRebuildLock(repoRoot string) (func(), error) { + if repoRoot == "" { + // Without a known root, just no-op the lock. Callers that + // rely on it for safety (the CLI command path) always set + // RepoRoot; tests that don't can opt out. + return func() {}, nil + } + path := filepath.Join(repoRoot, rebuildLockFileName) + file, err := os.OpenFile(path, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600) + if err != nil { + if errors.Is(err, fs.ErrExist) { + holder := "unknown" + if data, readErr := os.ReadFile(path); readErr == nil { + holder = strings.TrimSpace(string(data)) + } + return nil, fmt.Errorf( + "another integration rebuild is in progress (held by pid %s); "+ + "if it has exited unexpectedly, remove %s and retry", + holder, path) + } + return nil, fmt.Errorf("acquire rebuild lock: %w", err) + } + _, _ = fmt.Fprintln(file, os.Getpid()) + _ = file.Close() + return func() { + if err := os.Remove(path); err != nil && !errors.Is(err, fs.ErrNotExist) { + // Best-effort: a leftover lock blocks future rebuilds, + // but we cannot do better here without panicking. + _ = err + } + }, nil +} + // shouldAutoResolve resolves opts against DefaultAutoResolve. func (h *Handler) shouldAutoResolve(opts *RebuildOptions) bool { if opts != nil && opts.AutoResolve != nil { @@ -565,6 +721,27 @@ func (h *Handler) resumeRebuild( pending *state.IntegrationRebuild, opts *RebuildOptions, ) (*RebuildResult, error) { + // Pending state can advance past the last tip when an earlier + // rebuild's auto-resolve loop committed the final merge but the + // process was killed (or beaten to it by a concurrent run) + // before the post-loop cleanup cleared the pending entry. In + // that case there is genuinely nothing left to merge — just + // clear the stale entry and report it explicitly so the user + // doesn't see the meaningless "Resuming at tip N+1 of N" log. + if pending.NextTipIndex >= len(pending.Tips) { + h.Log.Infof( + "Discarding pending rebuild state: all %d tip(s) already merged", + len(pending.Tips)) + if err := h.Store.ClearPendingIntegrationRebuild(ctx); err != nil { + h.Log.Warnf("clear pending rebuild: %v", err) + } + hashes := make([]git.Hash, len(pending.Tips)) + for i, t := range pending.Tips { + hashes[i] = t.Hash + } + return &RebuildResult{Name: info.Name, TipHashes: hashes}, nil + } + clean, err := h.Worktree.IsClean(ctx) if err != nil { return nil, fmt.Errorf("check worktree: %w", err) @@ -609,6 +786,11 @@ func (h *Handler) runMerges( // succeeds, gs hands the deduped list to the Regenerator (if any). var regenPaths []string + enableRerere := opts == nil || !opts.NoRerere + if !enableRerere { + h.Log.Info("Rerere replay and recording disabled for this rebuild") + } + for i := start; i < len(tips); i++ { tip := tips[i] mergeMsg := fmt.Sprintf("Merge %s into %s", tip.Name, info.Name) @@ -623,15 +805,28 @@ func (h *Handler) runMerges( _ = logFile.Close() mergeEnv := []string{regenLogEnvVar + "=" + logPath} + var rerereReplays []string + onRerereReplay := func(path string) { + rerereReplays = append(rerereReplays, path) + } + err = h.Worktree.Merge(ctx, git.MergeOptions{ - Refs: []string{tip.Hash.String()}, - NoFF: true, - Message: mergeMsg, - EnableRerere: true, - LeaveConflict: true, - Env: mergeEnv, + Refs: []string{tip.Hash.String()}, + NoFF: true, + Message: mergeMsg, + EnableRerere: enableRerere, + LeaveConflict: true, + Env: mergeEnv, + OnRerereReplay: onRerereReplay, }) + if len(rerereReplays) > 0 { + h.Log.Infof( + "Tip %q: rerere replayed cached resolution for %d path(s): %s", + tip.Name, len(rerereReplays), + strings.Join(rerereReplays, ", ")) + } + // Always drain + clean up the log file, regardless of merge // outcome. regenPaths = appendRegenLog(regenPaths, logPath) @@ -648,13 +843,51 @@ func (h *Handler) runMerges( // Try the auto-resolver if enabled and configured. if h.shouldAutoResolve(opts) && h.Resolver != nil { + h.Log.Infof( + "Tip %q: invoking resolver for %d conflicted path(s): %s", + tip.Name, len(conflict.ConflictPaths), + strings.Join(conflict.ConflictPaths, ", ")) resolved, resolveErr := h.autoResolveLoop( ctx, info.Name, tip.Name, conflict.ConflictPaths, mergeMsg) switch { case resolveErr != nil: - h.Log.Warnf("Auto-resolve failed for tip %q: %v", - tip.Name, resolveErr) - // fall through to accept-incoming / conflict-surfacing + // Both branches save pending state and return so the + // rebuild can be resumed after the user addresses the + // underlying issue. Accept-incoming is bypassed in + // both cases — silently picking "theirs" is exactly + // the failure mode that motivated this whole flow. + if saveErr := h.Store.SetPendingIntegrationRebuild(ctx, &state.IntegrationRebuild{ + Integration: info.Name, + Tips: tips, + NextTipIndex: i + 1, + }); saveErr != nil { + h.Log.Warnf("save pending rebuild: %v", saveErr) + } + + // Structural "give up" from the resolver: valid JSON + // with unresolved_files and no questions. The + // resolver did its job; surface the conflict for + // manual resolution. + if errors.Is(resolveErr, errResolverUnresolved) { + h.Log.Warnf( + "Tip %q: resolver could not resolve: %v", + tip.Name, resolveErr) + return nil, &ConflictError{ + Tip: tip.Name, + Paths: conflict.ConflictPaths, + } + } + + // Corrupt or unusable resolver response (parse error, + // non-zero exit, missing markers, iteration cap). + // Halt with a distinct error so the caller knows the + // prompt/model/script needs attention rather than + // just a manual merge. + return nil, &ResolverFailedError{ + Tip: tip.Name, + Paths: conflict.ConflictPaths, + Cause: resolveErr, + } case resolved: continue } @@ -670,7 +903,10 @@ func (h *Handler) runMerges( tip.Name, acceptErr) // fall through to conflict-surfacing } else { - h.Log.Infof("Tip %q: accepted incoming for %d conflicted path(s): %s", + h.Log.Warnf( + "Tip %q: accepted incoming for %d conflicted path(s) "+ + "(may have dropped declarations from the integration "+ + "side): %s", tip.Name, len(conflict.ConflictPaths), strings.Join(conflict.ConflictPaths, ", ")) continue @@ -694,10 +930,26 @@ func (h *Handler) runMerges( // written real updates that must not be left dirty in the worktree, and // `git commit --amend --no-edit --allow-empty` is a safe no-op when no // changes are present. + // + // regeneratorErr is preserved on [RebuildResult.RegeneratorError] so + // the CLI surfaces it alongside the success summary; "rebuilt with N + // tips" silently hiding a broken regen output is exactly what + // confused users in earlier runs. + var regeneratorErr error deduped := dedupStrings(regenPaths) if h.Regenerator != nil && len(deduped) > 0 { + h.Log.Infof( + "Invoking regenerator for %d take-incoming path(s): %s", + len(deduped), strings.Join(deduped, ", ")) if err := h.Regenerator.Regenerate(ctx, deduped); err != nil { - h.Log.Warnf("regenerator: %v", err) + regeneratorErr = err + h.Log.Errorf( + "Regenerator failed: %v. Integration branch still "+ + "contains the merged source, but its generated "+ + "files may be out of sync — your build will need "+ + "a manual 'mise run generate' (or your "+ + "project equivalent) before it compiles.", + err) } if err := h.Worktree.AmendCommitAll(ctx); err != nil { return nil, fmt.Errorf("amend with regen output: %w", err) @@ -723,8 +975,9 @@ func (h *Handler) runMerges( hashes[i] = t.Hash } return &RebuildResult{ - Name: info.Name, - TipHashes: hashes, + Name: info.Name, + TipHashes: hashes, + RegeneratorError: regeneratorErr, }, nil } @@ -1044,7 +1297,7 @@ func (h *Handler) autoResolveLoop( if len(resp.UnresolvedFiles) > 0 { return false, fmt.Errorf( - "resolver reported unresolved files with no questions: %s", + "%w: %s", errResolverUnresolved, strings.Join(resp.UnresolvedFiles, ", ")) } @@ -1060,6 +1313,35 @@ func (h *Handler) autoResolveLoop( maxIters) } +// resetRerereCache deletes the rerere cache directory inside the +// git dir. Subsequent rerere recording during the rebuild repopulates +// it from scratch — that's the point: stale bad postimages from a +// prior run are gone, but rerere is still on so good resolutions +// from this rebuild get cached for the next one. +// +// Missing cache directory is a no-op (rerere may never have run). +func (h *Handler) resetRerereCache() error { + path := filepath.Join(h.Repository.GitDir(), "rr-cache") + if err := os.RemoveAll(path); err != nil { + return fmt.Errorf("remove %s: %w", path, err) + } + h.Log.Infof("Rerere cache cleared: %s", path) + return nil +} + +// removeResolutionFile deletes the resolution file under .spice/ +// if one exists. Missing root or missing file are no-ops. +func (h *Handler) removeResolutionFile() error { + if h.RepoRoot == "" { + return nil + } + path := spicedir.ResolutionPath(h.RepoRoot, ResolutionFeatureName) + if err := os.Remove(path); err != nil && !errors.Is(err, fs.ErrNotExist) { + return fmt.Errorf("remove %s: %w", path, err) + } + return nil +} + // appendQAToFile appends the given question/answer pairs to the // resolution file's entry for (ours, theirs). func (h *Handler) appendQAToFile( diff --git a/internal/handler/integration/handler_test.go b/internal/handler/integration/handler_test.go index fcc96e4d..ec69328f 100644 --- a/internal/handler/integration/handler_test.go +++ b/internal/handler/integration/handler_test.go @@ -842,8 +842,8 @@ func TestHandler_Rebuild_autoResolveResolverError(t *testing.T) { _, err := h.Rebuild(t.Context(), &RebuildOptions{AutoResolve: boolPtr(true)}) require.Error(t, err) - var conflictErr *ConflictError - require.True(t, errors.As(err, &conflictErr)) + var resolverFail *ResolverFailedError + require.ErrorAs(t, err, &resolverFail) } func TestHandler_Rebuild_autoResolveDisabledByOpts(t *testing.T) { @@ -896,8 +896,8 @@ func TestHandler_Rebuild_autoResolveIterationCap(t *testing.T) { _, err := h.Rebuild(t.Context(), &RebuildOptions{AutoResolve: boolPtr(true)}) require.Error(t, err) - var conflictErr *ConflictError - require.True(t, errors.As(err, &conflictErr)) + var resolverFail *ResolverFailedError + require.ErrorAs(t, err, &resolverFail) } func TestHandler_Rebuild_autoResolveAssumptions(t *testing.T) { @@ -955,36 +955,28 @@ func TestHandler_Rebuild_acceptIncomingFallback(t *testing.T) { assert.Equal(t, "preview", res.Name) } -func TestHandler_Rebuild_acceptIncomingAfterResolverFailure(t *testing.T) { +func TestHandler_Rebuild_haltOnResolverFailure(t *testing.T) { h, mocks, resolver, _ := newHandlerWithResolver(t) - mergeMsg := setupConflictMerge(t, mocks) + _ = setupConflictMerge(t, mocks) resolver.EXPECT(). Resolve(gomock.Any(), gomock.Any()). Return(nil, errors.New("resolver crashed")) - mocks.Worktree.EXPECT(). - CheckoutTheirs(gomock.Any(), []string{"shared.txt"}). - Return(nil) - mocks.Worktree.EXPECT(). - MergeContinue(gomock.Any(), []string{"shared.txt"}, mergeMsg). - Return(nil) - - mocks.Store.EXPECT(). - SetIntegration(gomock.Any(), gomock.Any()). - Return(nil) mocks.Store.EXPECT(). - ClearPendingIntegrationRebuild(gomock.Any()). - Return(nil) - mocks.Worktree.EXPECT(). - CheckoutBranch(gomock.Any(), "main"). + SetPendingIntegrationRebuild(gomock.Any(), gomock.Any()). Return(nil) _, err := h.Rebuild(t.Context(), &RebuildOptions{ AutoResolve: boolPtr(true), AcceptIncoming: boolPtr(true), }) - require.NoError(t, err) + require.Error(t, err) + var resolverFail *ResolverFailedError + require.ErrorAs(t, err, &resolverFail) + assert.Equal(t, "feat-a", resolverFail.Tip) + assert.Equal(t, []string{"shared.txt"}, resolverFail.Paths) + assert.ErrorContains(t, resolverFail.Cause, "resolver crashed") } func TestHandler_Rebuild_acceptIncomingDisabled(t *testing.T) { diff --git a/internal/handler/integration/mocks_test.go b/internal/handler/integration/mocks_test.go index cc1ca1d2..92daa8a6 100644 --- a/internal/handler/integration/mocks_test.go +++ b/internal/handler/integration/mocks_test.go @@ -44,6 +44,44 @@ func (m *MockGitRepository) EXPECT() *MockGitRepositoryMockRecorder { return m.recorder } +// GitDir mocks base method. +func (m *MockGitRepository) GitDir() string { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GitDir") + ret0, _ := ret[0].(string) + return ret0 +} + +// GitDir indicates an expected call of GitDir. +func (mr *MockGitRepositoryMockRecorder) GitDir() *MockGitRepositoryGitDirCall { + mr.mock.ctrl.T.Helper() + call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GitDir", reflect.TypeOf((*MockGitRepository)(nil).GitDir)) + return &MockGitRepositoryGitDirCall{Call: call} +} + +// MockGitRepositoryGitDirCall wrap *gomock.Call +type MockGitRepositoryGitDirCall struct { + *gomock.Call +} + +// Return rewrite *gomock.Call.Return +func (c *MockGitRepositoryGitDirCall) Return(arg0 string) *MockGitRepositoryGitDirCall { + c.Call = c.Call.Return(arg0) + return c +} + +// Do rewrite *gomock.Call.Do +func (c *MockGitRepositoryGitDirCall) Do(f func() string) *MockGitRepositoryGitDirCall { + c.Call = c.Call.Do(f) + return c +} + +// DoAndReturn rewrite *gomock.Call.DoAndReturn +func (c *MockGitRepositoryGitDirCall) DoAndReturn(f func() string) *MockGitRepositoryGitDirCall { + c.Call = c.Call.DoAndReturn(f) + return c +} + // ListRemoteRefs mocks base method. func (m *MockGitRepository) ListRemoteRefs(ctx context.Context, remote string, opts *git.ListRemoteRefsOptions) iter.Seq2[git.RemoteRef, error] { m.ctrl.T.Helper() diff --git a/internal/handler/integration/resolver.go b/internal/handler/integration/resolver.go index fefc19e5..267894b9 100644 --- a/internal/handler/integration/resolver.go +++ b/internal/handler/integration/resolver.go @@ -1,9 +1,11 @@ package integration import ( + "bytes" "context" "errors" "fmt" + "strings" "go.abhg.dev/gs/internal/scriptrun" "go.abhg.dev/gs/internal/silog" @@ -76,16 +78,51 @@ type ScriptResolveError struct { } func (e *ScriptResolveError) Error() string { + var msg string switch e.Stage { case "exit": - return fmt.Sprintf( - "resolver exited with code %d", e.ExitCode) + msg = fmt.Sprintf("resolver exited with code %d", e.ExitCode) case "parse": - return fmt.Sprintf( - "resolver output is not valid JSON: %v", e.Err) + msg = fmt.Sprintf("resolver output is not valid JSON: %v", e.Err) default: - return fmt.Sprintf("resolver failed: %v", e.Err) + msg = fmt.Sprintf("resolver failed: %v", e.Err) } + return msg + e.diagnosticPreview() +} + +// diagnosticPreview returns a truncated preview of the resolver's +// stdout and stderr for inclusion in the error message. Each stream +// is capped at scriptOutputPreviewBytes to keep the log readable; +// truncated streams are marked with a leading note. Empty streams +// are explicitly labeled so the user knows whether a stream was +// missing or unbounded. +func (e *ScriptResolveError) diagnosticPreview() string { + if len(e.Stdout) == 0 && len(e.Stderr) == 0 { + return "" + } + var b strings.Builder + b.WriteString("\n--- resolver stdout ") + writeOutputPreview(&b, e.Stdout) + b.WriteString("\n--- resolver stderr ") + writeOutputPreview(&b, e.Stderr) + return b.String() +} + +const scriptOutputPreviewBytes = 2048 + +func writeOutputPreview(b *strings.Builder, out []byte) { + if len(out) == 0 { + b.WriteString("(empty) ---") + return + } + if len(out) <= scriptOutputPreviewBytes { + b.WriteString("---\n") + b.Write(bytes.TrimRight(out, "\n")) + return + } + fmt.Fprintf(b, "(last %d of %d bytes) ---\n", + scriptOutputPreviewBytes, len(out)) + b.Write(bytes.TrimRight(out[len(out)-scriptOutputPreviewBytes:], "\n")) } func (e *ScriptResolveError) Unwrap() error { return e.Err } @@ -112,7 +149,7 @@ func (r *ScriptResolver) Resolve( Dir: r.RepoRoot, Env: scriptrun.EnvFor( scriptrun.OpIntegrationRebuild, - req.TipName, // GS_BRANCH = the tip being merged in + req.TipName, // GS_BRANCH = the tip being merged in req.IntegrationName, // GS_BASE = the integration branch ), }) diff --git a/stack_submit.go b/stack_submit.go index 52d9e827..63f7de5d 100644 --- a/stack_submit.go +++ b/stack_submit.go @@ -128,4 +128,3 @@ func (cmd *stackSubmitCmd) runFromIntegration( } return err } - diff --git a/testdata/help/integration_rebuild.txt b/testdata/help/integration_rebuild.txt index b4940987..9ffa5570 100644 --- a/testdata/help/integration_rebuild.txt +++ b/testdata/help/integration_rebuild.txt @@ -15,19 +15,53 @@ resolver script is invoked to attempt automatic resolution before surfacing conflicts. See the recipe for details on the JSON protocol the script must implement. -Any conflicts that survive the merge drivers and the resolver are auto-resolved -by taking the incoming tip's version. Pass --no-accept-incoming (or set -spice.integration.acceptIncoming =false) to disable that final fallback and -surface conflicts for manual resolution instead. +If the resolver produces corrupt or unusable output (script exit failure, +malformed JSON, missing markers) the rebuild halts rather than falling through +to accept-incoming. The usual cause is a prompt, model, or script issue that +needs to be fixed, not glossed over by silently picking 'theirs'. + +Conflicts that survive the merge drivers and a successful resolver run are +auto-resolved by taking the incoming tip's version. Pass --no-accept-incoming +(or set spice.integration.acceptIncoming=false) to disable that final fallback +and surface conflicts for manual resolution instead. + +If a bad resolution was silently cached (in rerere, in the resolution file, +or in pending rebuild state) and is being replayed on every rebuild, +use --reset-rerere-cache to wipe stale postimages (while still recording fresh +ones), --reset-resolution-file to wipe the Q&A history, --reset-pending to drop +any stale resume point, or --from-scratch for all three. + +--no-rerere is a stronger diagnostic mode: it disables rerere recording too, so +the rebuild produces nothing cached for next time. Use it only when you suspect +rerere itself is misbehaving — otherwise --reset-rerere-cache is what you want. Flags: - --push Also push the integration branch after rebuilding - --[no-]auto-resolve Auto-resolve merge conflicts using the configured - resolver script (🔧 spice.integration.autoResolve) - --[no-]accept-incoming Final-stage fallback: take the incoming tip's - version for any remaining conflicts so the - rebuild completes without manual intervention (🔧 - spice.integration.acceptIncoming) + --push Also push the integration branch after rebuilding + --[no-]auto-resolve Auto-resolve merge conflicts using the configured + resolver script (🔧 spice.integration.autoResolve) + --[no-]accept-incoming Final-stage fallback: take the incoming tip's + version for any remaining conflicts so the + rebuild completes without manual intervention (🔧 + spice.integration.acceptIncoming) + --no-rerere Disable rerere entirely (no replay AND no + recording) for this rebuild. Diagnostic mode — + prefer --reset-rerere-cache when you want fresh + resolutions cached for next time. + --reset-rerere-cache Wipe the rerere cache (.git/rr-cache) before + starting so stale cached resolutions are not + replayed. Rerere stays enabled so the rebuild's + fresh resolutions are recorded. + --reset-resolution-file Delete the resolution file + (.integration_resolution.json) before starting so + stale Q&A history is not carried into this rebuild. + --reset-pending Clear any pending rebuild state before starting. + Use when a prior halted rebuild left stale state + that should not be resumed. + --from-scratch Shorthand: implies --reset-rerere-cache, + --reset-resolution-file, and --reset-pending. + Use after a bad rebuild left stale state in any + cache; rerere stays enabled so good resolutions get + recorded. Global Flags: -h, --help Show help for the command