diff --git a/README.md b/README.md index c5421c8..bc5d433 100644 --- a/README.md +++ b/README.md @@ -27,8 +27,8 @@ This repository contains the first local alpha kernel. It supports: - Isolated detached worktrees per attempt - Immutable checkpoints and proposals - Checks bound to exact source trees -- Conservative path-level conflict detection -- Three-tree composition of disjoint proposals +- Real three-way content merging, including compatible same-file edits +- Agent-ready reconciliation workspaces for genuine merge conflicts - Optional validation on the final integrated tree - Compare-and-swap acceptance - Safe visible-root materialization on Desktop landing @@ -64,7 +64,7 @@ The user continues typing into Codex Desktop normally. The skill invokes `hop be ## Build -Requires Go 1.26+ and Git. +Requires Go 1.26+ and Git 2.40+. ```bash go build -o hop ./cmd/hop @@ -105,7 +105,7 @@ hop propose --summary "Added password reset emails" P_... hop land R_... -- go test ./... ``` -The command supplied to `land` runs in a temporary worktree containing the exact final tree that would become accepted. If it fails, `refs/hop/accepted` and the SQLite accepted head do not move. After acceptance, Hop updates only the visible working files through a disposable Git index; HEAD, the active branch, and the real index do not move. The agent pauses for an explicit review-first request, failed validation, proposal overlap, visible-root divergence, or newly required destructive/external scope. +The command supplied to `land` runs in a temporary worktree containing the exact final tree that would become accepted. If it fails, `refs/hop/accepted` and the SQLite accepted head do not move. After acceptance, Hop updates only the visible working files through a disposable Git index; HEAD, the active branch, and the real index do not move. Compatible edits to the same file are merged automatically. A genuine merge conflict becomes a fresh agent reconciliation workspace, which the skill resolves, checks, and lands without asking the user to manage source-control mechanics. The agent pauses only for an explicit review-first request, failed validation, visible-root divergence, unresolved product intent, or newly required destructive/external scope. `land` and `accept` are intentionally different: @@ -162,9 +162,15 @@ hop start --agent codex "Add a health endpoint" hop start --agent claude "Add an account empty state" ``` -If their changed paths are disjoint, both proposals can land in either order. The second proposal is composed onto the latest accepted tree and may be validated there. +If their changed paths are disjoint, both proposals can land in either order. The second proposal is composed onto the latest accepted tree and validated there. -If both proposals changed the same path since their shared base, Hop blocks the stale proposal even when Git could textually merge it. The proposal remains addressable for review or a future reconciliation prompt. +If both proposals changed the same file, Hop performs a real three-way merge. +Independent hunks and identical changes land automatically. Only genuine merge +conflicts pause acceptance; `hop land` prepares a fresh reconciliation +workspace and the agent resolves, retests, reproposes, and lands it +automatically. Text conflicts use diff3 markers; structural and binary +conflicts may not. The visible project root remains at the last accepted state +until that resolution passes. ## State model @@ -217,7 +223,11 @@ The JSON shape is an alpha contract and may evolve before the first tagged relea ## Safety boundary -Hop currently treats any shared changed path as a conflict. Disjoint files can still conflict behaviorally, so automatic acceptance reruns the strongest relevant validation on the exact final tree. Manual acceptance remains available as an explicit review-first mode. +Hop uses Git's three-way content merge rather than treating shared paths as +conflicts. Textual cleanliness still cannot prove behavioral compatibility, so +automatic acceptance reruns the strongest relevant validation on the exact +final tree. Manual acceptance remains available as an explicit review-first +mode. Desktop landing fails closed when nonignored visible source does not exactly match an accepted Hop state or when an ignored/untracked path would be overwritten. It never uses `reset --hard`, moves HEAD, or rewrites the user's real index. The lower-level `hop accept` command deliberately retains controller-only behavior and does not update the visible root. diff --git a/docs/product-blueprint.md b/docs/product-blueprint.md index dc96003..46249e2 100644 --- a/docs/product-blueprint.md +++ b/docs/product-blueprint.md @@ -195,9 +195,14 @@ Hop should distinguish several kinds of interaction: Non-overlapping files are not sufficient evidence of compatibility. Database migrations, API callers and implementations, dependency upgrades, generated outputs, and shared invariants often conflict across files. -“Semantic merge” should not initially mean that a model silently rewrites conflicting code. The safer operation is **refresh**: +“Semantic merge” should not mean blindly rewriting conflicting code. Hop first +uses deterministic Git three-way merging. When genuine hunks remain, the agent +automatically enters a provenance-linked **refresh** workspace: -> Give the original agent the newer accepted state, the intervening task summaries, the conflict packet, and its original intent; ask it to produce a new proposal. +> Give the original agent the newer accepted state, the intervening task +> summaries, the conflict-marker tree, and its original intent; require it to +> resolve, validate, repropose, and land without asking the human to coordinate +> an ordinary code merge. For many agent tasks, replaying or regenerating against the latest state is cheaper and safer than preserving every old hunk. diff --git a/internal/hop/cli.go b/internal/hop/cli.go index e24dd53..784cdba 100644 --- a/internal/hop/cli.go +++ b/internal/hop/cli.go @@ -22,6 +22,7 @@ Usage: hop propose [--summary TEXT] STATE hop accept STATE [-- COMMAND [ARG...]] hop land STATE [-- COMMAND [ARG...]] + hop refresh PROPOSAL hop sync hop status hop graph @@ -38,7 +39,7 @@ Usage: Add --json anywhere for machine-readable output. ` -const Version = "0.1.0-alpha.3" +const Version = "0.1.0-alpha.4" func RunCLI(args []string, stdout, stderr io.Writer) int { return RunCLIWithInput(args, os.Stdin, stdout, stderr) @@ -266,6 +267,28 @@ func RunCLIWithInput(args []string, stdin io.Reader, stdout, stderr io.Writer) i result, err := service.Land(ctx, stateID, argv) value = result if err != nil { + var conflict *ConflictError + if errors.As(err, &conflict) { + refresh, refreshErr := service.Refresh(ctx, stateID) + if refreshErr != nil { + preparationErr := fmt.Errorf("automatic merge conflict detected (%v), but reconciliation preparation failed: %v", err, refreshErr) + return printCLIError(preparationErr, jsonOutput, stdout, stderr) + } + if jsonOutput { + writeJSON(stdout, map[string]any{ + "ok": false, + "error": err.Error(), + "exit_code": 20, + "conflict": conflict, + "reconciliation": refresh, + "next_command": "resolve the returned workspace, then check, propose, and land using the returned prompt state", + }) + return 20 + } + fmt.Fprintf(stderr, "hop: %v\n", err) + printRefreshSummary(stdout, refresh) + return 20 + } return printCLIError(err, jsonOutput, stdout, stderr) } if !jsonOutput { @@ -279,6 +302,20 @@ func RunCLIWithInput(args []string, stdin io.Reader, stdout, stderr io.Writer) i } } + case "refresh", "reconcile": + if len(commandArgs) != 1 { + fmt.Fprintln(stderr, "usage: hop refresh PROPOSAL") + return 2 + } + result, err := service.Refresh(ctx, commandArgs[0]) + value = result + if err != nil { + return printCLIError(err, jsonOutput, stdout, stderr) + } + if !jsonOutput { + printRefreshSummary(stdout, result) + } + case "sync": if len(commandArgs) != 0 { fmt.Fprintln(stderr, "usage: hop sync") @@ -529,7 +566,7 @@ func printCLIError(err error, jsonOutput bool, stdout, stderr io.Writer) int { } else { fmt.Fprintf(stderr, "hop: %v\n", err) if conflict != nil && len(conflict.Paths) > 0 { - fmt.Fprintln(stderr, "Overlapping paths:") + fmt.Fprintln(stderr, "Merge conflicts:") for _, path := range conflict.Paths { fmt.Fprintf(stderr, " %s\n", path) } @@ -544,6 +581,23 @@ func printCLIError(err error, jsonOutput bool, stdout, stderr io.Writer) int { return code } +func printRefreshSummary(w io.Writer, result RefreshResult) { + action := "Prepared" + if result.Reused { + action = "Reusing" + } + fmt.Fprintf(w, "%s reconciliation prompt %s\n", action, result.Prompt.ID) + fmt.Fprintf(w, "Reconciliation attempt: %s\n", result.Attempt.ID) + fmt.Fprintf(w, "Workspace: %s\n", result.Workspace) + fmt.Fprintf(w, "Accepted input: %s · commit %s\n", result.AcceptedHead.ID, shortHash(result.AcceptedHead.GitCommit)) + fmt.Fprintf(w, "Proposal input: %s · commit %s\n", result.Proposal.ID, shortHash(result.Proposal.GitCommit)) + fmt.Fprintln(w, "Resolve these genuine merge conflicts while preserving both intents (structural conflicts may have no text markers):") + for _, path := range result.Conflicts { + fmt.Fprintf(w, " %s\n", path) + } + fmt.Fprintf(w, "Continue automatically with: hop check %s -- , then propose and land again.\n", result.Prompt.ID) +} + func removeFlag(args []string, wanted string) (bool, []string) { found := false filtered := make([]string, 0, len(args)) diff --git a/internal/hop/db.go b/internal/hop/db.go index 66ec674..767f83e 100644 --- a/internal/hop/db.go +++ b/internal/hop/db.go @@ -559,6 +559,126 @@ func (s *Store) CreateTaskAttemptPrompt( return task, attempt, prompt, nil } +// CreateAttemptPrompt atomically adds a new attempt and its initial prompt to +// an existing task. Reconciliation uses a fresh workspace so a frozen proposal +// and any concurrent work in its original attempt are never overwritten. +func (s *Store) CreateAttemptPrompt( + ctx context.Context, + attempt Attempt, + prompt State, + parents []Parent, +) (Attempt, State, error) { + now := time.Now().UTC() + if attempt.ID == "" { + attempt.ID = newID("at") + } + if attempt.TaskID == "" { + return Attempt{}, State{}, errors.New("hop: reconciliation attempt requires a task ID") + } + if attempt.BaseStateID == "" { + return Attempt{}, State{}, errors.New("hop: reconciliation attempt requires a base state") + } + if strings.TrimSpace(attempt.Workspace) == "" { + return Attempt{}, State{}, errors.New("hop: reconciliation attempt requires a workspace") + } + if attempt.CreatedAt.IsZero() { + attempt.CreatedAt = now + } + if attempt.Status == "" { + attempt.Status = "reconciling" + } + if prompt.ID == "" { + prompt.ID = newID("p") + } + if prompt.Kind == "" { + prompt.Kind = StatePrompt + } + if prompt.Kind != StatePrompt { + return Attempt{}, State{}, fmt.Errorf("hop: attempt instruction must have kind %q", StatePrompt) + } + if prompt.TaskID == "" { + prompt.TaskID = attempt.TaskID + } + if prompt.TaskID != attempt.TaskID { + return Attempt{}, State{}, errors.New("hop: prompt task does not match reconciliation attempt") + } + if prompt.AttemptID == "" { + prompt.AttemptID = attempt.ID + } + if prompt.AttemptID != attempt.ID { + return Attempt{}, State{}, errors.New("hop: prompt attempt does not match reconciliation attempt") + } + if prompt.Agent == "" { + prompt.Agent = attempt.Agent + } + if prompt.CreatedAt.IsZero() { + prompt.CreatedAt = now + } + + tx, err := s.db.BeginTx(ctx, nil) + if err != nil { + return Attempt{}, State{}, fmt.Errorf("begin reconciliation attempt creation: %w", err) + } + defer tx.Rollback() + + var taskExists int + if err := tx.QueryRowContext(ctx, `SELECT 1 FROM tasks WHERE id = ?`, attempt.TaskID).Scan(&taskExists); err != nil { + return Attempt{}, State{}, dbNotFound("task", attempt.TaskID, err) + } + base, err := stateTx(ctx, tx, attempt.BaseStateID) + if err != nil { + return Attempt{}, State{}, fmt.Errorf("read reconciliation base state: %w", err) + } + if prompt.CanonicalAnchorID == "" { + prompt.CanonicalAnchorID = attempt.BaseStateID + } + if prompt.SourceTree == "" { + prompt.SourceTree = base.SourceTree + } + if prompt.GitCommit == "" { + prompt.GitCommit = base.GitCommit + } + parents = chooseParents(prompt.Parents, parents) + parents = ensureParentRole(parents, "run_parent", attempt.BaseStateID) + parents = ensureParentRole(parents, "canonical_anchor", prompt.CanonicalAnchorID) + parents = canonicalizeParents(parents) + prompt.Parents = parents + attempt.HeadStateID = prompt.ID + + if _, err := tx.ExecContext(ctx, + `INSERT INTO attempts(id, task_id, agent, workspace, base_state_id, head_state_id, status, created_at) + VALUES (?, ?, ?, ?, ?, NULL, ?, ?)`, + attempt.ID, attempt.TaskID, attempt.Agent, attempt.Workspace, attempt.BaseStateID, + attempt.Status, formatTime(attempt.CreatedAt)); err != nil { + return Attempt{}, State{}, fmt.Errorf("insert reconciliation attempt: %w", err) + } + if err := insertStateTx(ctx, tx, prompt, parents); err != nil { + return Attempt{}, State{}, err + } + if _, err := tx.ExecContext(ctx, + `UPDATE attempts SET head_state_id = ? WHERE id = ?`, prompt.ID, attempt.ID); err != nil { + return Attempt{}, State{}, fmt.Errorf("install reconciliation attempt head: %w", err) + } + if _, err := tx.ExecContext(ctx, + `UPDATE tasks SET status = 'reconciling' WHERE id = ?`, attempt.TaskID); err != nil { + return Attempt{}, State{}, fmt.Errorf("mark task reconciling: %w", err) + } + if err := insertEventTx(ctx, tx, Event{ + Kind: "attempt.created", TaskID: attempt.TaskID, AttemptID: attempt.ID, + }, map[string]string{"purpose": "reconciliation"}); err != nil { + return Attempt{}, State{}, err + } + if err := insertEventTx(ctx, tx, Event{ + Kind: "prompt.created", TaskID: attempt.TaskID, AttemptID: attempt.ID, StateID: prompt.ID, + }, map[string]string{"purpose": "reconciliation"}); err != nil { + return Attempt{}, State{}, err + } + if err := tx.Commit(); err != nil { + return Attempt{}, State{}, fmt.Errorf("commit reconciliation attempt creation: %w", err) + } + return attempt, prompt, nil +} + // AppendState atomically appends an immutable state and compare-and-swaps the // owning attempt's head. An empty expectedAttemptHeadID means "the head observed // by this transaction"; callers that already observed a head should pass it. @@ -829,6 +949,66 @@ func (s *Store) AcceptedForProposal(ctx context.Context, proposalID string) (Sta return state, true, nil } +// ReconciliationPrompt returns the prompt already created to reconcile a +// proposal against a particular accepted head. The pair is unique by +// convention and makes `hop refresh` safe to retry. +func (s *Store) ReconciliationPrompt(ctx context.Context, proposalID, acceptedID string) (State, bool, error) { + var id string + err := s.db.QueryRowContext(ctx, + `SELECT s.id + FROM states s + JOIN state_parents source + ON source.state_id = s.id + AND source.parent_state_id = ? + AND source.role = 'reconciliation_source' + JOIN state_parents anchor + ON anchor.state_id = s.id + AND anchor.parent_state_id = ? + AND anchor.role = 'canonical_anchor' + WHERE s.kind = 'prompt' + ORDER BY s.created_at DESC, s.id DESC + LIMIT 1`, proposalID, acceptedID).Scan(&id) + if errors.Is(err, sql.ErrNoRows) { + return State{}, false, nil + } + if err != nil { + return State{}, false, fmt.Errorf("find reconciliation prompt: %w", err) + } + state, err := s.GetState(ctx, id) + if err != nil { + return State{}, false, err + } + return state, true, nil +} + +// ReconciliationPromptForAttempt identifies attempts created specifically to +// resolve a prior proposal. It lets proposal creation enforce reconciliation +// evidence even when the caller names a later checkpoint instead of the +// attempt's initial prompt. +func (s *Store) ReconciliationPromptForAttempt(ctx context.Context, attemptID string) (State, bool, error) { + var id string + err := s.db.QueryRowContext(ctx, + `SELECT s.id + FROM states s + JOIN state_parents source + ON source.state_id = s.id + AND source.role = 'reconciliation_source' + WHERE s.attempt_id = ? AND s.kind = 'prompt' + ORDER BY s.created_at, s.id + LIMIT 1`, attemptID).Scan(&id) + if errors.Is(err, sql.ErrNoRows) { + return State{}, false, nil + } + if err != nil { + return State{}, false, fmt.Errorf("find reconciliation prompt for attempt %s: %w", attemptID, err) + } + state, err := s.GetState(ctx, id) + if err != nil { + return State{}, false, err + } + return state, true, nil +} + // MaterializedHead is the accepted state currently projected into the visible // project root. It may trail AcceptedHead after controller-only acceptance or // an interrupted post-acceptance synchronization. diff --git a/internal/hop/git.go b/internal/hop/git.go index 52fe25a..6e6e327 100644 --- a/internal/hop/git.go +++ b/internal/hop/git.go @@ -131,7 +131,7 @@ func (s *GitStore) Ensure(ctx context.Context, path string) (*Repository, error) if info, statErr := os.Stat(path); statErr == nil && !info.IsDir() { if root, findErr := s.FindRoot(ctx, filepath.Dir(path)); findErr == nil { - return s.openRoot(ctx, root, true) + return s.openRootForEnsure(ctx, root) } return nil, fmt.Errorf("cannot initialize a repository at non-directory %q", path) } else if statErr != nil && !errors.Is(statErr, os.ErrNotExist) { @@ -139,18 +139,46 @@ func (s *GitStore) Ensure(ctx context.Context, path string) (*Repository, error) } if root, findErr := s.FindRoot(ctx, path); findErr == nil { - return s.openRoot(ctx, root, true) + return s.openRootForEnsure(ctx, root) } if err := os.MkdirAll(path, 0o755); err != nil { return nil, fmt.Errorf("create repository directory: %w", err) } + lockPath, err := repositoryInitLockPath(path) + if err != nil { + return nil, err + } + release, err := acquireFileLock(ctx, lockPath, "Hop repository initialization") + if err != nil { + return nil, err + } + defer release() + + // A concurrent Hop process may have initialized this directory while this + // process waited for the per-user bootstrap lock. + if root, findErr := s.FindRoot(ctx, path); findErr == nil { + return s.openRoot(ctx, root, true) + } if _, err := s.run(ctx, path, nil, nil, "init", "--quiet", path); err != nil { return nil, err } return s.openRoot(ctx, path, true) } +func (s *GitStore) openRootForEnsure(ctx context.Context, root string) (*Repository, error) { + lockPath, err := repositoryInitLockPath(root) + if err != nil { + return nil, err + } + release, err := acquireFileLock(ctx, lockPath, "Hop repository initialization") + if err != nil { + return nil, err + } + defer release() + return s.openRoot(ctx, root, true) +} + // Open opens the non-bare repository containing path. func (s *GitStore) Open(ctx context.Context, path string) (*Repository, error) { root, err := s.FindRoot(ctx, path) @@ -293,6 +321,15 @@ func (r *Repository) SnapshotWithOptions(ctx context.Context, options SnapshotOp if _, err := r.run(ctx, env, nil, "add", "-A", "--", "."); err != nil { return "", "", fmt.Errorf("snapshot workspace: %w", err) } + // The disposable index is copied from the worktree index and therefore + // carries its stat cache. An agent can rewrite a file to the same size within + // the filesystem timestamp granularity, making a plain `git add -A` + // intermittently treat fresh content as unchanged. Renormalizing forces Git + // to hash every tracked path again while retaining additions/deletions staged + // by the first pass. + if _, err := r.run(ctx, env, nil, "add", "--renormalize", "--", "."); err != nil { + return "", "", fmt.Errorf("rehash workspace snapshot: %w", err) + } treeOutput, err := r.run(ctx, env, nil, "write-tree") if err != nil { return "", "", fmt.Errorf("write snapshot tree: %w", err) @@ -880,55 +917,85 @@ func (r *Repository) TrackedPaths(ctx context.Context, path string) ([]string, e return paths, nil } -// ComposeTrees conservatively composes the changes base->theirs onto ours. -// It uses Git's three-tree index merge, so a path independently changed to -// different content is reported as a conflict instead of being rewritten. No -// worktree or user index is read or changed. On conflict, tree is empty and the -// sorted conflicting path set is returned with a nil error. +// ComposeTrees performs Git's real three-way merge without touching a +// worktree or index. Independent hunks in the same file, identical changes, +// renames, and compatible mode/content changes merge automatically. On a +// genuine conflict, tree is the best-effort conflict-marker tree and conflicts +// lists the paths an agent must reconcile. func (r *Repository) ComposeTrees(ctx context.Context, base, ours, theirs string) (tree string, conflicts []string, err error) { - baseTree, err := r.resolveTree(ctx, base) + baseCommit, err := r.resolveCommit(ctx, base) if err != nil { - return "", nil, fmt.Errorf("resolve base tree: %w", err) + return "", nil, fmt.Errorf("resolve base commit: %w", err) } - oursTree, err := r.resolveTree(ctx, ours) + oursCommit, err := r.resolveCommit(ctx, ours) if err != nil { - return "", nil, fmt.Errorf("resolve current tree: %w", err) + return "", nil, fmt.Errorf("resolve current commit: %w", err) } - theirsTree, err := r.resolveTree(ctx, theirs) + theirsCommit, err := r.resolveCommit(ctx, theirs) if err != nil { - return "", nil, fmt.Errorf("resolve proposal tree: %w", err) + return "", nil, fmt.Errorf("resolve proposal commit: %w", err) } - indexPath, cleanup, err := r.temporaryIndex(false) - if err != nil { - return "", nil, err + output, mergeErr := r.run(ctx, []string{"GIT_OPTIONAL_LOCKS=0"}, nil, + "-c", "merge.conflictStyle=zdiff3", + "-c", "merge.renames=true", + "-c", "merge.directoryRenames=conflict", + "merge-tree", "--write-tree", "--merge-base", baseCommit, + "--name-only", "-z", "--no-messages", oursCommit, theirsCommit, + ) + fields := splitNull([]byte(output)) + if len(fields) == 0 || fields[0] == "" { + if mergeErr != nil { + return "", nil, fmt.Errorf("compose trees: %w", mergeErr) + } + return "", nil, errors.New("git merge-tree returned no tree") } - defer cleanup() - env := []string{"GIT_INDEX_FILE=" + indexPath, "GIT_OPTIONAL_LOCKS=0"} - // --aggressive resolves only additional provably-trivial cases (for example, - // a deletion on one side while the other side is unchanged). It does not - // attempt a content merge of independently modified files. - _, mergeErr := r.run(ctx, env, nil, "read-tree", "-m", "--aggressive", baseTree, oursTree, theirsTree) - - unmergedOutput, listErr := r.run(ctx, env, nil, "ls-files", "-u", "-z") - if listErr == nil { - conflicts = parseUnmergedPaths([]byte(unmergedOutput)) + tree = fields[0] + if err := validObjectName(tree); err != nil { + return "", nil, fmt.Errorf("invalid composed tree: %w", err) } - if len(conflicts) > 0 { - return "", conflicts, nil + conflictSet := map[string]struct{}{} + for _, path := range fields[1:] { + if path != "" { + conflictSet[path] = struct{}{} + } } - if mergeErr != nil { + for path := range conflictSet { + conflicts = append(conflicts, path) + } + sort.Strings(conflicts) + if mergeErr == nil { + if len(conflicts) > 0 { + return "", nil, errors.New("git merge-tree reported conflict paths with a successful exit") + } + return tree, nil, nil + } + if gitExitCode(mergeErr) != 1 { return "", nil, fmt.Errorf("compose trees: %w", mergeErr) } - if listErr != nil { - return "", nil, fmt.Errorf("inspect composed index: %w", listErr) + if len(conflicts) == 0 { + // Git documents conflict classes (notably some directory-renames) that + // produce no conflicted-file list. Give the agent the changed-path union + // as useful reconciliation candidates instead of converting a real merge + // conflict into an internal error. + for _, side := range []string{oursCommit, theirsCommit} { + paths, pathsErr := r.ChangedPaths(ctx, baseCommit, side) + if pathsErr != nil { + return "", nil, fmt.Errorf("derive structural conflict paths: %w", pathsErr) + } + for _, path := range paths { + conflictSet[path] = struct{}{} + } + } + for path := range conflictSet { + conflicts = append(conflicts, path) + } + sort.Strings(conflicts) + if len(conflicts) == 0 { + conflicts = []string{"(structural merge conflict; inspect both inputs)"} + } } - - treeOutput, err := r.run(ctx, env, nil, "write-tree") - if err != nil { - return "", nil, fmt.Errorf("write composed tree: %w", err) - } - return trimLine(treeOutput), nil, nil + return tree, conflicts, nil } // VerifyObject verifies that name resolves to an object in this repository. @@ -1002,6 +1069,17 @@ func (r *Repository) resolveTree(ctx context.Context, object string) (string, er return trimLine(output), nil } +func (r *Repository) resolveCommit(ctx context.Context, object string) (string, error) { + if err := validObjectName(object); err != nil { + return "", err + } + output, err := r.run(ctx, nil, nil, "rev-parse", "--verify", "--quiet", object+"^{commit}") + if err != nil { + return "", fmt.Errorf("resolve commit %s: %w", object, err) + } + return trimLine(output), nil +} + func (r *Repository) validateRef(ctx context.Context, ref string) error { if !strings.HasPrefix(ref, "refs/") { return fmt.Errorf("ref must be fully qualified: %q", ref) diff --git a/internal/hop/model.go b/internal/hop/model.go index 6af8616..2187784 100644 --- a/internal/hop/model.go +++ b/internal/hop/model.go @@ -96,6 +96,19 @@ type SyncResult struct { Changed bool `json:"changed"` } +type RefreshResult struct { + Prompt State `json:"prompt"` + Task Task `json:"task"` + Attempt Attempt `json:"attempt"` + Proposal State `json:"proposal"` + AcceptedHead State `json:"accepted_head"` + Workspace string `json:"workspace"` + Deliver []string `json:"deliver"` + ConflictTree string `json:"conflict_tree"` + Conflicts []string `json:"conflicts"` + Reused bool `json:"reused"` +} + type PromptRedaction struct { Kind string `json:"kind"` Count int `json:"count"` @@ -170,11 +183,11 @@ func (e *CommittedStateError) Error() string { func (e *CommittedStateError) Unwrap() error { return e.Err } type ConflictError struct { - Paths []string + Paths []string `json:"paths"` } func (e *ConflictError) Error() string { - return "proposal overlaps changes accepted since its canonical anchor" + return "automatic three-way merge has genuine unresolved conflicts" } type RootConflictError struct { diff --git a/internal/hop/service.go b/internal/hop/service.go index ce8d1b5..73a7acb 100644 --- a/internal/hop/service.go +++ b/internal/hop/service.go @@ -2,6 +2,7 @@ package hop import ( "context" + "encoding/json" "errors" "fmt" "os" @@ -11,7 +12,10 @@ import ( "time" ) -const acceptedRef = "accepted" +const ( + acceptedRef = "accepted" + reconciliationSummaryPrefix = "hop-reconciliation-conflicts:" +) type Service struct { Root string @@ -36,6 +40,11 @@ func InitProject(ctx context.Context, path string) (*Service, State, error) { if err := os.MkdirAll(filepath.Join(hopDir, "workspaces"), 0o755); err != nil { return nil, State{}, fmt.Errorf("create Hop project directory: %w", err) } + releaseInit, err := acquireProjectLock(ctx, root, "init") + if err != nil { + return nil, State{}, err + } + defer releaseInit() store, err := OpenStore(filepath.Join(hopDir, "hop.db")) if err != nil { return nil, State{}, err @@ -52,6 +61,9 @@ func InitProject(ctx context.Context, path string) (*Service, State, error) { service.Close() return nil, State{}, err } + // Derived-ref repair acquires accept.lock. Release init.lock first so a + // validation child opening this project cannot invert those locks. + releaseInit() if err := service.reconcileDerivedRefs(ctx, true); err != nil { service.Close() return nil, State{}, err @@ -99,6 +111,11 @@ func OpenProject(start string) (*Service, error) { if err != nil { return nil, err } + releaseInit, err := acquireProjectLock(context.Background(), root, "init") + if err != nil { + return nil, err + } + defer releaseInit() store, err := OpenStore(filepath.Join(root, ".hop", "hop.db")) if err != nil { return nil, err @@ -128,6 +145,8 @@ func OpenProject(start string) (*Service, error) { } service := &Service{Root: root, Store: store, Repo: repo} reconcileAccepted := os.Getenv("HOP_ACCEPTANCE_LOCK_HELD") != "1" + // Never hold init.lock while derived-ref repair acquires accept.lock. + releaseInit() if err := service.reconcileDerivedRefs(context.Background(), reconcileAccepted); err != nil { service.Close() return nil, err @@ -459,6 +478,18 @@ func (s *Service) Propose(ctx context.Context, stateID, summary string) (Proposa if err != nil { return ProposalResult{}, err } + reconciliationPrompt, reconciliation, err := s.Store.ReconciliationPromptForAttempt(ctx, attempt.ID) + if err != nil { + return ProposalResult{}, err + } + var reconciliationConflicts []string + if reconciliation { + conflicts, ok := decodeReconciliationConflicts(reconciliationPrompt.Summary) + if !ok { + return ProposalResult{}, fmt.Errorf("reconciliation prompt %s has invalid conflict metadata", reconciliationPrompt.ID) + } + reconciliationConflicts = conflicts + } workspaceRepo, err := OpenRepository(attempt.Workspace) if err != nil { return ProposalResult{}, err @@ -467,17 +498,46 @@ func (s *Service) Propose(ctx context.Context, stateID, summary string) (Proposa if err != nil { return ProposalResult{}, err } + if reconciliation { + unresolved, err := unresolvedReconciliationMarkers(ctx, workspaceRepo, tree, reconciliationConflicts) + if err != nil { + return ProposalResult{}, err + } + if len(unresolved) > 0 { + return ProposalResult{}, fmt.Errorf("reconciliation still contains merge markers in: %s", strings.Join(unresolved, ", ")) + } + } + checks, err := s.Store.ListChecks(ctx, attempt.ID, tree) + if err != nil { + return ProposalResult{}, err + } + if reconciliation { + validated := false + for _, check := range checks { + if check.ExitCode == 0 { + validated = true + break + } + } + if !validated { + return ProposalResult{}, fmt.Errorf("reconciliation must pass hop check on the resolved tree before proposing") + } + } if strings.TrimSpace(summary) == "" { summary = task.Title } summary, _ = RedactPromptSecrets(summary) + canonicalAnchorID := attempt.BaseStateID + if reconciliation && reconciliationPrompt.CanonicalAnchorID != "" { + canonicalAnchorID = reconciliationPrompt.CanonicalAnchorID + } parents := canonicalizeParents([]Parent{{StateID: attempt.HeadStateID, Role: "run_parent", Order: 0}}) proposal := State{ ID: newID("r"), Kind: StateProposal, TaskID: attempt.TaskID, AttemptID: attempt.ID, - CanonicalAnchorID: attempt.BaseStateID, + CanonicalAnchorID: canonicalAnchorID, SourceTree: tree, GitCommit: commit, Summary: summary, @@ -498,10 +558,6 @@ func (s *Service) Propose(ctx context.Context, stateID, summary string) (Proposa } _ = s.Store.UpdateAttemptStatus(ctx, attempt.ID, "proposed") _ = s.Store.UpdateTaskStatus(ctx, task.ID, "proposed") - checks, err := s.Store.ListChecks(ctx, attempt.ID, tree) - if err != nil { - return ProposalResult{}, err - } return ProposalResult{Proposal: proposal, Checks: checks}, nil } @@ -547,7 +603,11 @@ func (s *Service) accept(ctx context.Context, proposalID string, checkCommand [] if err != nil { return AcceptResult{}, err } - base, err := s.Store.GetState(ctx, attempt.BaseStateID) + baseStateID := proposal.CanonicalAnchorID + if baseStateID == "" { + baseStateID = attempt.BaseStateID + } + base, err := s.Store.GetState(ctx, baseStateID) if err != nil { return AcceptResult{}, err } @@ -563,10 +623,6 @@ func (s *Service) accept(ctx context.Context, proposalID string, checkCommand [] if err != nil { return AcceptResult{}, err } - overlap := intersectPaths(proposalPaths, currentPaths) - if len(overlap) > 0 { - return AcceptResult{}, &ConflictError{Paths: overlap} - } finalTree := proposal.SourceTree if current.SourceTree != base.SourceTree { var mergeConflicts []string @@ -727,6 +783,210 @@ func (s *Service) Sync(ctx context.Context) (SyncResult, error) { return s.syncLocked(ctx) } +// Refresh prepares an agent-editable three-way conflict tree in the original +// attempt workspace. It appends an internal reconciliation prompt so the agent +// can resolve genuine conflicts without asking the user to coordinate them. +func (s *Service) Refresh(ctx context.Context, proposalID string) (RefreshResult, error) { + release, err := acquireProjectLock(ctx, s.Root, "accept") + if err != nil { + return RefreshResult{}, err + } + defer release() + + proposal, err := s.Store.GetState(ctx, proposalID) + if err != nil { + return RefreshResult{}, err + } + if proposal.Kind != StateProposal { + return RefreshResult{}, fmt.Errorf("state %s is %s, not a proposal", proposal.ID, proposal.Kind) + } + sourceAttempt, err := s.Store.GetAttempt(ctx, proposal.AttemptID) + if err != nil { + return RefreshResult{}, err + } + task, err := s.Store.GetTask(ctx, proposal.TaskID) + if err != nil { + return RefreshResult{}, err + } + current, err := s.Store.AcceptedHead(ctx) + if err != nil { + return RefreshResult{}, err + } + if existing, exists, err := s.Store.ReconciliationPrompt(ctx, proposal.ID, current.ID); err != nil { + return RefreshResult{}, err + } else if exists { + return s.reconciliationResult(ctx, existing, task, proposal, current, true) + } + baseStateID := proposal.CanonicalAnchorID + if baseStateID == "" { + baseStateID = sourceAttempt.BaseStateID + } + base, err := s.Store.GetState(ctx, baseStateID) + if err != nil { + return RefreshResult{}, err + } + conflictTree, conflicts, err := s.Repo.ComposeTrees(ctx, base.GitCommit, current.GitCommit, proposal.GitCommit) + if err != nil { + return RefreshResult{}, err + } + if len(conflicts) == 0 { + return RefreshResult{}, fmt.Errorf("proposal %s now merges cleanly; retry hop land", proposal.ID) + } + commit, err := s.Repo.CommitTree(ctx, conflictTree, []string{current.GitCommit, proposal.GitCommit}, + fmt.Sprintf("Reconcile %s against %s\n", proposal.ID, current.ID)) + if err != nil { + return RefreshResult{}, err + } + summary, err := encodeReconciliationConflicts(conflicts) + if err != nil { + return RefreshResult{}, err + } + instruction := fmt.Sprintf( + "Resolve proposal %s (%s) against accepted state %s (%s). Preserve both compatible intents. Inspect both input states for structural, delete/rename, mode, symlink, or binary conflicts that may have no text markers; resolve every conflict intentionally, remove all merge markers, run hop check, propose the result, and land it without asking the user to coordinate the merge. Conflict candidates: %s", + proposal.ID, proposal.Summary, current.ID, current.Summary, strings.Join(conflicts, ", ")) + instruction, _ = RedactPromptSecrets(instruction) + attemptID := newID("at") + workspace := filepath.Join(s.Root, ".hop", "workspaces", attemptID) + reconciliationAttempt := Attempt{ + ID: attemptID, + TaskID: proposal.TaskID, + Agent: sourceAttempt.Agent, + Workspace: workspace, + BaseStateID: current.ID, + Status: "reconciling", + CreatedAt: time.Now().UTC(), + } + parents := canonicalizeParents([]Parent{ + {StateID: proposal.ID, Role: "run_parent", Order: 0}, + {StateID: current.ID, Role: "canonical_anchor", Order: 1}, + {StateID: proposal.ID, Role: "reconciliation_source", Order: 2}, + }) + prompt := State{ + ID: newID("p"), + Kind: StatePrompt, + TaskID: proposal.TaskID, + AttemptID: reconciliationAttempt.ID, + CanonicalAnchorID: current.ID, + SourceTree: conflictTree, + GitCommit: commit, + Prompt: instruction, + Summary: summary, + Agent: reconciliationAttempt.Agent, + CreatedAt: time.Now().UTC(), + Parents: parents, + } + prompt.Digest, err = digestState(prompt, parents) + if err != nil { + return RefreshResult{}, err + } + if err := s.pinState(ctx, prompt); err != nil { + return RefreshResult{}, err + } + promptPinned := true + defer func() { + if promptPinned { + _ = s.Repo.DeleteRef(context.Background(), "refs/hop/states/"+prompt.ID, prompt.GitCommit) + } + }() + if err := os.MkdirAll(filepath.Dir(workspace), 0o755); err != nil { + return RefreshResult{}, fmt.Errorf("create reconciliation workspace directory: %w", err) + } + if _, err := s.Repo.AddDetachedWorktree(ctx, workspace, prompt.GitCommit); err != nil { + _ = s.Repo.RemoveWorktree(context.Background(), workspace, true) + return RefreshResult{}, fmt.Errorf("create reconciliation workspace: %w", err) + } + workspaceInstalled := true + defer func() { + if workspaceInstalled { + _ = s.Repo.RemoveWorktree(context.Background(), workspace, true) + } + }() + reconciliationAttempt, prompt, err = s.Store.CreateAttemptPrompt(ctx, reconciliationAttempt, prompt, parents) + if err != nil { + return RefreshResult{}, err + } + workspaceInstalled = false + promptPinned = false + task.Status = "reconciling" + return s.reconciliationResult(ctx, prompt, task, proposal, current, false) +} + +func (s *Service) reconciliationResult( + ctx context.Context, + prompt State, + task Task, + proposal State, + current State, + reused bool, +) (RefreshResult, error) { + conflicts, ok := decodeReconciliationConflicts(prompt.Summary) + if !ok || len(conflicts) == 0 { + return RefreshResult{}, fmt.Errorf("reconciliation prompt %s has no conflict metadata", prompt.ID) + } + attempt, err := s.Store.GetAttempt(ctx, prompt.AttemptID) + if err != nil { + return RefreshResult{}, err + } + if _, err := OpenRepository(attempt.Workspace); err != nil { + return RefreshResult{}, fmt.Errorf("open prepared reconciliation workspace: %w", err) + } + task, err = s.Store.GetTask(ctx, task.ID) + if err != nil { + return RefreshResult{}, err + } + return RefreshResult{ + Prompt: prompt, + Task: task, + Attempt: attempt, + Proposal: proposal, + AcceptedHead: current, + Workspace: attempt.Workspace, + Deliver: s.deliveryEnvironment(prompt, attempt), + ConflictTree: prompt.SourceTree, + Conflicts: conflicts, + Reused: reused, + }, nil +} + +func encodeReconciliationConflicts(paths []string) (string, error) { + encoded, err := json.Marshal(paths) + if err != nil { + return "", fmt.Errorf("encode reconciliation conflicts: %w", err) + } + return reconciliationSummaryPrefix + string(encoded), nil +} + +func decodeReconciliationConflicts(summary string) ([]string, bool) { + if !strings.HasPrefix(summary, reconciliationSummaryPrefix) { + return nil, false + } + var paths []string + if err := json.Unmarshal([]byte(strings.TrimPrefix(summary, reconciliationSummaryPrefix)), &paths); err != nil { + return nil, false + } + return paths, true +} + +func unresolvedReconciliationMarkers(ctx context.Context, repo *Repository, tree string, conflicts []string) ([]string, error) { + args := []string{ + "grep", "-z", "-l", "-I", + "-e", "^<<<<<<< ", + "-e", "^>>>>>>> ", + tree, "--", + } + args = append(args, conflicts...) + output, err := repo.run(ctx, []string{"GIT_LITERAL_PATHSPECS=1", "GIT_OPTIONAL_LOCKS=0"}, nil, args...) + if err != nil { + if gitExitCode(err) == 1 { + return nil, nil + } + return nil, fmt.Errorf("inspect reconciliation markers: %w", err) + } + unresolved := splitNull([]byte(output)) + sort.Strings(unresolved) + return unresolved, nil +} + func (s *Service) syncLocked(ctx context.Context) (SyncResult, error) { current, err := s.Store.AcceptedHead(ctx) if err != nil { diff --git a/internal/hop/service_smoke_test.go b/internal/hop/service_smoke_test.go index a3f9f45..cbd0ad2 100644 --- a/internal/hop/service_smoke_test.go +++ b/internal/hop/service_smoke_test.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "os" "os/exec" "path/filepath" @@ -81,6 +82,171 @@ func TestInitRefusesAUserTrackedHopDirectory(t *testing.T) { } } +func TestConcurrentFirstBeginsInitializeExactlyOnce(t *testing.T) { + t.Setenv("HOP_ROOT", "") + root := t.TempDir() + writeTestFile(t, filepath.Join(root, "base.txt"), "base\n") + previousDirectory, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + if err := os.Chdir(root); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chdir(previousDirectory) }) + + type result struct { + code int + stdout string + stderr string + } + const workers = 8 + start := make(chan struct{}) + results := make(chan result, workers) + var group sync.WaitGroup + for index := 0; index < workers; index++ { + group.Add(1) + go func(index int) { + defer group.Done() + <-start + var stdout, stderr bytes.Buffer + code := RunCLI([]string{ + "begin", "--json", "--agent", fmt.Sprintf("agent-%d", index), + "--session", fmt.Sprintf("session-%d", index), fmt.Sprintf("Prompt %d", index), + }, &stdout, &stderr) + results <- result{code: code, stdout: stdout.String(), stderr: stderr.String()} + }(index) + } + close(start) + group.Wait() + close(results) + + var workspaces []string + for result := range results { + if result.code != 0 { + t.Fatalf("concurrent first begin exited %d\nstdout: %s\nstderr: %s", result.code, result.stdout, result.stderr) + } + var response map[string]any + if err := json.Unmarshal([]byte(result.stdout), &response); err != nil { + t.Fatalf("decode concurrent begin output %q: %v", result.stdout, err) + } + data := objectField(t, response, "data") + workspaces = append(workspaces, stringField(t, data, "workspace")) + } + for _, workspace := range workspaces { + if info, err := os.Stat(workspace); err != nil || !info.IsDir() { + t.Fatalf("concurrent begin workspace %q missing: %v", workspace, err) + } + } + + service, err := OpenProject(root) + if err != nil { + t.Fatal(err) + } + defer service.Close() + graph, err := service.Store.Graph(context.Background(), "") + if err != nil { + t.Fatal(err) + } + initialAccepted := 0 + prompts := 0 + for _, row := range graph { + if row.State.Kind == StateAccepted && row.State.TaskID == "" { + initialAccepted++ + } + if row.State.Kind == StatePrompt { + prompts++ + } + } + if initialAccepted != 1 || prompts != workers { + t.Fatalf("concurrent bootstrap created %d initial states and %d prompts, want 1 and %d", initialAccepted, prompts, workers) + } +} + +func TestConcurrentInitInExistingGitRepository(t *testing.T) { + root := t.TempDir() + runGitTest(t, root, "init", "--quiet") + writeTestFile(t, filepath.Join(root, "base.txt"), "base\n") + + const workers = 8 + start := make(chan struct{}) + type result struct { + stateID string + err error + } + results := make(chan result, workers) + var group sync.WaitGroup + for range workers { + group.Add(1) + go func() { + defer group.Done() + <-start + service, initial, err := InitProject(context.Background(), root) + if service != nil { + _ = service.Close() + } + results <- result{stateID: initial.ID, err: err} + }() + } + close(start) + group.Wait() + close(results) + + initialID := "" + for result := range results { + if result.err != nil { + t.Fatalf("concurrent existing-repository init: %v", result.err) + } + if initialID == "" { + initialID = result.stateID + } else if result.stateID != initialID { + t.Fatalf("concurrent init returned initial states %s and %s", initialID, result.stateID) + } + } + exclude, err := os.ReadFile(filepath.Join(root, ".git", "info", "exclude")) + if err != nil { + t.Fatal(err) + } + if count := strings.Count(string(exclude), ".hop/\n"); count != 1 { + t.Fatalf("concurrent initialization wrote .hop/ exclusion %d times", count) + } +} + +func TestOpenProjectWaitsForInitializationLock(t *testing.T) { + service, _ := newTestProject(t, map[string]string{"base.txt": "base\n"}) + root := service.Root + if err := service.Close(); err != nil { + t.Fatal(err) + } + release, err := acquireProjectLock(context.Background(), root, "init") + if err != nil { + t.Fatal(err) + } + opened := make(chan error, 1) + go func() { + project, openErr := OpenProject(root) + if openErr == nil { + openErr = project.Close() + } + opened <- openErr + }() + select { + case err := <-opened: + release() + t.Fatalf("OpenProject returned before initialization lock release: %v", err) + case <-time.After(100 * time.Millisecond): + } + release() + select { + case err := <-opened: + if err != nil { + t.Fatal(err) + } + case <-time.After(5 * time.Second): + t.Fatal("OpenProject did not resume after initialization lock release") + } +} + func TestCLIJSONWorkflow(t *testing.T) { t.Setenv("HOP_ROOT", "") root := t.TempDir() @@ -222,6 +388,76 @@ func TestDoctorRejectsCommitTreeAndDigestMismatch(t *testing.T) { } } +func TestCLILandConflictReturnsAutomaticReconciliation(t *testing.T) { + t.Setenv("HOP_ROOT", "") + ctx := context.Background() + service, _ := newTestProject(t, map[string]string{"shared.txt": "base\n"}) + first, err := service.CreatePrompt(ctx, "First", "", "one") + if err != nil { + t.Fatal(err) + } + second, err := service.CreatePrompt(ctx, "Second", "", "two") + if err != nil { + t.Fatal(err) + } + writeTestFile(t, filepath.Join(first.Workspace, "shared.txt"), "first\n") + writeTestFile(t, filepath.Join(second.Workspace, "shared.txt"), "second\n") + firstProposal, err := service.Propose(ctx, first.Prompt.ID, "First") + if err != nil { + t.Fatal(err) + } + secondProposal, err := service.Propose(ctx, second.Prompt.ID, "Second") + if err != nil { + t.Fatal(err) + } + if _, err := service.Land(ctx, firstProposal.Proposal.ID, nil); err != nil { + t.Fatal(err) + } + root := service.Root + if err := service.Close(); err != nil { + t.Fatal(err) + } + previousDirectory, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + if err := os.Chdir(root); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chdir(previousDirectory) }) + + var stdout, stderr bytes.Buffer + code := RunCLI([]string{"land", secondProposal.Proposal.ID, "--json"}, &stdout, &stderr) + if code != 20 { + t.Fatalf("land exit = %d, want 20\nstdout: %s\nstderr: %s", code, stdout.String(), stderr.String()) + } + var response map[string]any + if err := json.Unmarshal(stdout.Bytes(), &response); err != nil { + t.Fatal(err) + } + if response["next_command"] == nil { + t.Fatalf("conflict response omitted next command: %s", stdout.String()) + } + reconciliation := objectField(t, response, "reconciliation") + prompt := objectField(t, reconciliation, "prompt") + if stringField(t, prompt, "id") == "" || stringField(t, reconciliation, "workspace") == "" { + t.Fatalf("reconciliation response omitted prompt/workspace: %s", stdout.String()) + } + if status := stringField(t, objectField(t, reconciliation, "task"), "status"); status != "reconciling" { + t.Fatalf("reconciliation task status = %q, want reconciling", status) + } + if status := stringField(t, objectField(t, reconciliation, "attempt"), "status"); status != "reconciling" { + t.Fatalf("reconciliation attempt status = %q, want reconciling", status) + } + conflicts, ok := reconciliation["conflicts"].([]any) + if !ok || len(conflicts) != 1 || conflicts[0] != "shared.txt" { + t.Fatalf("conflicts = %#v", reconciliation["conflicts"]) + } + if contents, err := os.ReadFile(filepath.Join(root, "shared.txt")); err != nil || string(contents) != "first\n" { + t.Fatalf("conflicted land changed visible root: %q, %v", string(contents), err) + } +} + func TestOpenProjectInsideFinalValidationDoesNotReacquireAcceptanceLock(t *testing.T) { ctx := context.Background() service, _ := newTestProject(t, map[string]string{"base.txt": "base\n"}) @@ -479,6 +715,332 @@ func TestConcurrentDisjointAcceptancesSerialize(t *testing.T) { } } +func TestOverlappingSameFileIndependentHunksAutoMerge(t *testing.T) { + ctx := context.Background() + service, _ := newTestProject(t, map[string]string{ + "shared.txt": "header\nmiddle\nfooter\n", + }) + first, err := service.CreatePrompt(ctx, "Change header", "", "one") + if err != nil { + t.Fatal(err) + } + second, err := service.CreatePrompt(ctx, "Change footer", "", "two") + if err != nil { + t.Fatal(err) + } + writeTestFile(t, filepath.Join(first.Workspace, "shared.txt"), "HEADER\nmiddle\nfooter\n") + writeTestFile(t, filepath.Join(second.Workspace, "shared.txt"), "header\nmiddle\nFOOTER\n") + firstProposal, err := service.Propose(ctx, first.Prompt.ID, "Header") + if err != nil { + t.Fatal(err) + } + secondProposal, err := service.Propose(ctx, second.Prompt.ID, "Footer") + if err != nil { + t.Fatal(err) + } + if _, err := service.Accept(ctx, firstProposal.Proposal.ID, nil); err != nil { + t.Fatal(err) + } + merged, err := service.Accept(ctx, secondProposal.Proposal.ID, []string{ + "sh", "-c", `grep -qx HEADER shared.txt && grep -qx FOOTER shared.txt`, + }) + if err != nil { + var failed *CheckFailedError + if errors.As(err, &failed) { + contents, showErr := service.Repo.run(ctx, nil, nil, "show", failed.Check.TreeHash+":shared.txt") + t.Fatalf("mergeable same-file proposal failed validation with shared.txt=%q (show error %v): %v", contents, showErr, err) + } + t.Fatalf("mergeable same-file proposal was blocked: %v", err) + } + assertTreeFiles(t, service, merged.State.GitCommit, map[string]string{ + "shared.txt": "HEADER\nmiddle\nFOOTER\n", + }) + if len(merged.ProposalPaths) != 1 || len(merged.CurrentPaths) != 1 || + merged.ProposalPaths[0] != "shared.txt" || merged.CurrentPaths[0] != "shared.txt" { + t.Fatalf("overlap audit paths were not retained: proposal=%v current=%v", merged.ProposalPaths, merged.CurrentPaths) + } +} + +func TestSnapshotCapturesRacySameSizeRewrite(t *testing.T) { + ctx := context.Background() + service, _ := newTestProject(t, map[string]string{"shared.txt": "lower\n"}) + runGitTest(t, service.Root, "config", "core.trustctime", "false") + runGitTest(t, service.Root, "config", "core.checkStat", "minimal") + prompt, err := service.CreatePrompt(ctx, "Uppercase the value", "", "agent") + if err != nil { + t.Fatal(err) + } + path := filepath.Join(prompt.Workspace, "shared.txt") + before, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + writeTestFile(t, path, "UPPER\n") + if err := os.Chtimes(path, before.ModTime(), before.ModTime()); err != nil { + t.Fatal(err) + } + proposal, err := service.Propose(ctx, prompt.Prompt.ID, "Uppercase") + if err != nil { + t.Fatal(err) + } + assertTreeFiles(t, service, proposal.Proposal.GitCommit, map[string]string{"shared.txt": "UPPER\n"}) +} + +func TestSameAttemptFollowupCoalescesAlreadyAcceptedEdit(t *testing.T) { + ctx := context.Background() + service, initial := newTestProject(t, map[string]string{ + "hop-home.css": "body { color: black; }\n", + "footer.html": `` + "\n", + }) + first, err := service.CreatePrompt(ctx, "Add the primary color", "", "agent") + if err != nil { + t.Fatal(err) + } + writeTestFile(t, filepath.Join(first.Workspace, "hop-home.css"), "body { color: black; }\n:root { --color-primary: #724bdb; }\n") + firstProposal, err := service.Propose(ctx, first.Prompt.ID, "Add primary color") + if err != nil { + t.Fatal(err) + } + if _, err := service.Land(ctx, firstProposal.Proposal.ID, nil); err != nil { + t.Fatal(err) + } + + followup, err := service.CreatePrompt(ctx, "Bust the stylesheet cache", firstProposal.Proposal.ID, "agent") + if err != nil { + t.Fatal(err) + } + writeTestFile(t, filepath.Join(followup.Workspace, "footer.html"), ``+"\n") + secondProposal, err := service.Propose(ctx, followup.Prompt.ID, "Bump stylesheet cache key") + if err != nil { + t.Fatal(err) + } + if secondProposal.Proposal.CanonicalAnchorID != initial.ID { + t.Fatalf("test no longer exercises stale attempt base: proposal anchor = %s, want %s", secondProposal.Proposal.CanonicalAnchorID, initial.ID) + } + landed, err := service.Land(ctx, secondProposal.Proposal.ID, nil) + if err != nil { + t.Fatalf("same-attempt follow-up was blocked: %v", err) + } + assertTreeFiles(t, service, landed.State.GitCommit, map[string]string{ + "hop-home.css": "body { color: black; }\n:root { --color-primary: #724bdb; }\n", + "footer.html": `` + "\n", + }) +} + +func TestIdenticalSameFileChangesCoalesceAutomatically(t *testing.T) { + ctx := context.Background() + service, _ := newTestProject(t, map[string]string{"shared.css": "body {}\n"}) + first, err := service.CreatePrompt(ctx, "Add primary color", "", "one") + if err != nil { + t.Fatal(err) + } + second, err := service.CreatePrompt(ctx, "Add the same primary color", "", "two") + if err != nil { + t.Fatal(err) + } + want := "body {}\n:root { --primary: purple; }\n" + writeTestFile(t, filepath.Join(first.Workspace, "shared.css"), want) + writeTestFile(t, filepath.Join(second.Workspace, "shared.css"), want) + firstProposal, err := service.Propose(ctx, first.Prompt.ID, "Primary color") + if err != nil { + t.Fatal(err) + } + secondProposal, err := service.Propose(ctx, second.Prompt.ID, "Same primary color") + if err != nil { + t.Fatal(err) + } + if _, err := service.Accept(ctx, firstProposal.Proposal.ID, nil); err != nil { + t.Fatal(err) + } + secondAccepted, err := service.Accept(ctx, secondProposal.Proposal.ID, nil) + if err != nil { + t.Fatalf("identical same-file change was blocked: %v", err) + } + assertTreeFiles(t, service, secondAccepted.State.GitCommit, map[string]string{"shared.css": want}) +} + +func TestConcurrentSameFileCompatibleAcceptancesSerializeAndMerge(t *testing.T) { + ctx := context.Background() + service, _ := newTestProject(t, map[string]string{"shared.txt": "one\ntwo\nthree\n"}) + first, err := service.CreatePrompt(ctx, "Uppercase one", "", "one") + if err != nil { + t.Fatal(err) + } + second, err := service.CreatePrompt(ctx, "Uppercase three", "", "two") + if err != nil { + t.Fatal(err) + } + writeTestFile(t, filepath.Join(first.Workspace, "shared.txt"), "ONE\ntwo\nthree\n") + writeTestFile(t, filepath.Join(second.Workspace, "shared.txt"), "one\ntwo\nTHREE\n") + firstProposal, err := service.Propose(ctx, first.Prompt.ID, "One") + if err != nil { + t.Fatal(err) + } + secondProposal, err := service.Propose(ctx, second.Prompt.ID, "Three") + if err != nil { + t.Fatal(err) + } + errs := make(chan error, 2) + var wg sync.WaitGroup + for _, proposalID := range []string{firstProposal.Proposal.ID, secondProposal.Proposal.ID} { + wg.Add(1) + go func() { + defer wg.Done() + _, acceptErr := service.Accept(ctx, proposalID, nil) + errs <- acceptErr + }() + } + wg.Wait() + close(errs) + for err := range errs { + if err != nil { + t.Fatalf("concurrent compatible acceptance: %v", err) + } + } + head, err := service.Store.AcceptedHead(ctx) + if err != nil { + t.Fatal(err) + } + assertTreeFiles(t, service, head.GitCommit, map[string]string{ + "shared.txt": "ONE\ntwo\nTHREE\n", + }) +} + +func TestTrueConflictCreatesAgentReconciliationWorkspace(t *testing.T) { + ctx := context.Background() + service, _ := newTestProject(t, map[string]string{"shared.txt": "color=base\n"}) + first, err := service.CreatePrompt(ctx, "Use red", "", "one") + if err != nil { + t.Fatal(err) + } + second, err := service.CreatePrompt(ctx, "Use blue with fallback", "", "two") + if err != nil { + t.Fatal(err) + } + writeTestFile(t, filepath.Join(first.Workspace, "shared.txt"), "color=red\n") + writeTestFile(t, filepath.Join(second.Workspace, "shared.txt"), "color=blue\n") + firstProposal, err := service.Propose(ctx, first.Prompt.ID, "Red") + if err != nil { + t.Fatal(err) + } + secondProposal, err := service.Propose(ctx, second.Prompt.ID, "Blue") + if err != nil { + t.Fatal(err) + } + if _, err := service.Land(ctx, firstProposal.Proposal.ID, nil); err != nil { + t.Fatal(err) + } + _, err = service.Land(ctx, secondProposal.Proposal.ID, nil) + var conflict *ConflictError + if !errors.As(err, &conflict) { + t.Fatalf("land error = %v, want ConflictError", err) + } + refresh, err := service.Refresh(ctx, secondProposal.Proposal.ID) + if err != nil { + t.Fatal(err) + } + if refresh.Task.ID != second.Task.ID { + t.Fatalf("reconciliation escaped original task: %#v", refresh) + } + if refresh.Attempt.ID == second.Attempt.ID || refresh.Attempt.BaseStateID != refresh.AcceptedHead.ID { + t.Fatalf("reconciliation did not receive a fresh attempt at the accepted head: %#v", refresh.Attempt) + } + if len(refresh.Conflicts) != 1 || refresh.Conflicts[0] != "shared.txt" { + t.Fatalf("conflicts = %#v", refresh.Conflicts) + } + contents, err := os.ReadFile(filepath.Join(refresh.Workspace, "shared.txt")) + if err != nil { + t.Fatal(err) + } + if !bytes.Contains(contents, []byte("<<<<<<< ")) || + !bytes.Contains(contents, []byte("=======")) || + !bytes.Contains(contents, []byte(">>>>>>> ")) { + t.Fatalf("reconciliation workspace lacks useful diff3 markers:\n%s", contents) + } + if _, err := service.Propose(ctx, refresh.Prompt.ID, "Unresolved"); err == nil || !strings.Contains(err.Error(), "merge markers") { + t.Fatalf("unresolved proposal error = %v", err) + } + writeTestFile(t, filepath.Join(refresh.Workspace, "shared.txt"), "color=red-blue-fallback\n") + if _, err := service.Propose(ctx, refresh.Prompt.ID, "Unchecked resolution"); err == nil || !strings.Contains(err.Error(), "must pass hop check") { + t.Fatalf("unchecked reconciliation proposal error = %v", err) + } + if _, err := service.RunCheck(ctx, refresh.Prompt.ID, []string{ + "sh", "-c", `test "$(cat shared.txt)" = "color=red-blue-fallback"`, + }); err != nil { + t.Fatal(err) + } + resolvedProposal, err := service.Propose(ctx, refresh.Prompt.ID, "Resolve both color intents") + if err != nil { + t.Fatal(err) + } + resolved, err := service.Land(ctx, resolvedProposal.Proposal.ID, []string{ + "sh", "-c", `test "$(cat shared.txt)" = "color=red-blue-fallback"`, + }) + if err != nil { + t.Fatal(err) + } + if contents, err := os.ReadFile(filepath.Join(service.Root, "shared.txt")); err != nil || string(contents) != "color=red-blue-fallback\n" { + t.Fatalf("visible resolution = %q, err=%v", string(contents), err) + } + if resolved.MaterializedRoot != service.Root { + t.Fatalf("resolved root = %q", resolved.MaterializedRoot) + } +} + +func TestMarkerlessModifyDeleteConflictRequiresCheckedResolution(t *testing.T) { + ctx := context.Background() + service, _ := newTestProject(t, map[string]string{"shared.txt": "base\n"}) + modify, err := service.CreatePrompt(ctx, "Modify the shared file", "", "modifier") + if err != nil { + t.Fatal(err) + } + remove, err := service.CreatePrompt(ctx, "Remove the shared file", "", "remover") + if err != nil { + t.Fatal(err) + } + writeTestFile(t, filepath.Join(modify.Workspace, "shared.txt"), "accepted modification\n") + if err := os.Remove(filepath.Join(remove.Workspace, "shared.txt")); err != nil { + t.Fatal(err) + } + modifyProposal, err := service.Propose(ctx, modify.Prompt.ID, "Modify") + if err != nil { + t.Fatal(err) + } + removeProposal, err := service.Propose(ctx, remove.Prompt.ID, "Remove") + if err != nil { + t.Fatal(err) + } + if _, err := service.Land(ctx, modifyProposal.Proposal.ID, nil); err != nil { + t.Fatal(err) + } + if _, err := service.Land(ctx, removeProposal.Proposal.ID, nil); err == nil { + t.Fatal("modify/delete proposal unexpectedly landed without reconciliation") + } + refresh, err := service.Refresh(ctx, removeProposal.Proposal.ID) + if err != nil { + t.Fatal(err) + } + if _, err := service.Propose(ctx, refresh.Prompt.ID, "Unchecked structural resolution"); err == nil || !strings.Contains(err.Error(), "must pass hop check") { + t.Fatalf("unchecked markerless reconciliation error = %v", err) + } + if err := os.Remove(filepath.Join(refresh.Workspace, "shared.txt")); err != nil && !errors.Is(err, os.ErrNotExist) { + t.Fatal(err) + } + if _, err := service.RunCheck(ctx, refresh.Prompt.ID, []string{"sh", "-c", "test ! -e shared.txt"}); err != nil { + t.Fatal(err) + } + resolvedProposal, err := service.Propose(ctx, refresh.Prompt.ID, "Intentionally remove shared file") + if err != nil { + t.Fatal(err) + } + if _, err := service.Land(ctx, resolvedProposal.Proposal.ID, []string{"sh", "-c", "test ! -e shared.txt"}); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(filepath.Join(service.Root, "shared.txt")); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("resolved visible root retained deleted file: %v", err) + } +} + func TestOverlappingProposalAndFailedFinalCheckDoNotMoveHead(t *testing.T) { ctx := context.Background() service, _ := newTestProject(t, map[string]string{"shared.txt": "base\n"}) diff --git a/internal/hop/util.go b/internal/hop/util.go index e98b9de..cf0894a 100644 --- a/internal/hop/util.go +++ b/internal/hop/util.go @@ -124,6 +124,13 @@ func truncateRecordedOutput(output string) string { func acquireProjectLock(ctx context.Context, root, name string) (func(), error) { path := filepath.Join(root, ".hop", name+".lock") + return acquireFileLock(ctx, path, "Hop "+name) +} + +func acquireFileLock(ctx context.Context, path, description string) (func(), error) { + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + return nil, fmt.Errorf("create %s lock directory: %w", description, err) + } for { file, err := os.OpenFile(path, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600) if err == nil { @@ -160,12 +167,25 @@ func acquireProjectLock(ctx context.Context, root, name string) (func(), error) } select { case <-ctx.Done(): - return nil, fmt.Errorf("wait for Hop %s lock: %w", name, ctx.Err()) + return nil, fmt.Errorf("wait for %s lock: %w", description, ctx.Err()) case <-time.After(50 * time.Millisecond): } } } +func repositoryInitLockPath(path string) (string, error) { + cache, err := os.UserCacheDir() + if err != nil { + return "", fmt.Errorf("locate user cache for repository initialization lock: %w", err) + } + canonical, err := filepath.EvalSymlinks(path) + if err != nil { + return "", fmt.Errorf("resolve repository initialization path: %w", err) + } + digest := sha256.Sum256([]byte(filepath.Clean(canonical))) + return filepath.Join(cache, "hop", "locks", "repository-"+hex.EncodeToString(digest[:])+".lock"), nil +} + func shellQuote(argv []string) string { quoted := make([]string, len(argv)) for i, arg := range argv { diff --git a/skills/hop/SKILL.md b/skills/hop/SKILL.md index c19c268..8ef238b 100644 --- a/skills/hop/SKILL.md +++ b/skills/hop/SKILL.md @@ -53,7 +53,9 @@ variable or secret-manager name instead. - Do not stage files. Hop captures every nonignored workspace change. - Give a subagent project-changing work only after creating a distinct Hop prompt/attempt for that delegation. -- Never silently merge overlapping proposals. +- Never discard either side of concurrent work. Let Hop perform its three-way + merge, then resolve only the genuine conflict hunks in the reconciliation + workspace it returns. Verify the captured state before making changes: @@ -79,14 +81,31 @@ hop status --json hop propose --summary "" ``` -6. Unless the user explicitly requested review-only mode, immediately accept - the proposal and validate the exact final tree: +6. Unless the user explicitly requested review-only mode, immediately land the + proposal and validate the exact final tree: ```bash hop land -- [args...] ``` -7. Report the accepted result, validation, and remaining risks. Keep internal + Same-file edits with compatible hunks merge automatically. + +7. If `hop land` reports a prepared reconciliation prompt/workspace, continue + immediately in that returned workspace. Do not stop or ask the user to + coordinate an ordinary code conflict: + + - adopt every returned `HOP_*` value and the fresh reconciliation workspace; + - inspect every conflict candidate plus both returned proposal/current + accepted states; compare their commits when a delete/rename, binary, mode, + symlink, or directory conflict has no text markers; + - resolve every conflict intelligently, preserving both compatible intents; + - remove all merge markers; + - run `hop check` with the returned prompt state (Hop requires checked + reconciliation evidence before it will accept a new proposal); + - create a new proposal and run `hop land` again; and + - repeat if accepted state raced forward again. + +8. Report the accepted result, validation, and remaining risks. Keep internal state and evidence IDs out of the normal response unless they help explain a failure or the user asks for them. Confirm that `hop land` reported the selected visible project root as synchronized. @@ -114,11 +133,16 @@ Stop before acceptance only when: - the user explicitly says `review first`, `proposal only`, `do not land`, or otherwise asks to approve the result before it is accepted; - validation fails; -- Hop reports overlap, a changed accepted head, or visible-root divergence; or +- Hop reports visible-root divergence; a conflict has genuine product ambiguity + that cannot be resolved from both recorded intents; or - acceptance would require a destructive, external, or out-of-scope action not authorized by the captured task. -On overlap or validation failure, preserve the proposal and report the block. +Ordinary textual overlap is not a reason to stop. Hop first performs a real +three-way content merge; genuine unresolved hunks enter the automatic +reconciliation loop above. Preserve and report a block only when the intents +are product-level incompatible, required validation cannot be repaired, or +safe continuation needs new user authority. If visible-root synchronization is blocked, do not bypass it with `hop accept`, force checkout, reset, or file copying. Preserve the proposal and identify the user-owned paths that must be resolved. `hop accept` is reserved for an diff --git a/skills/hop/agents/openai.yaml b/skills/hop/agents/openai.yaml index 25ea683..0089343 100644 --- a/skills/hop/agents/openai.yaml +++ b/skills/hop/agents/openai.yaml @@ -1,7 +1,7 @@ interface: display_name: "Hop Version Control" short_description: "Capture every coding prompt before project effects" - default_prompt: "Use $hop to capture this prompt first, complete the task in its isolated workspace, validate it, and auto-accept the proposal unless I explicitly request review first." + default_prompt: "Use $hop to capture this prompt first, complete and validate the task in its isolated workspace, auto-land it, and automatically reconcile ordinary merge conflicts unless I explicitly request review first." policy: allow_implicit_invocation: true diff --git a/skills/hop/references/protocol.md b/skills/hop/references/protocol.md index c4abd08..426af4f 100644 --- a/skills/hop/references/protocol.md +++ b/skills/hop/references/protocol.md @@ -68,6 +68,7 @@ hop status --json hop check "$HOP_STATE_ID" -- hop propose --summary "" "$HOP_STATE_ID" hop land -- +hop refresh ``` `hop check` snapshots the attempt and runs the command in a detached worktree materialized from that exact checkpoint. Edits made concurrently in the live workspace do not change the tested tree. @@ -77,10 +78,30 @@ hop land -- The initial task prompt authorizes the agent to run `hop land` after successful validation; a second user approval is not required. Manual review is an opt-in mode: stop at the proposal only when the user explicitly asks to review or -approve before acceptance. Validation failure, overlap, a stale accepted head, -or newly required destructive/external scope also stops automatic acceptance. +approve before acceptance. Validation failure, visible-root divergence, +unresolved product ambiguity, or newly required destructive/external scope +stops automatic acceptance. Path overlap and a stale accepted head do not: +Hop merges, retries, or prepares agent reconciliation. -`hop land` is the Desktop operation. It compares paths changed by the proposal with paths accepted since its base, validates and advances accepted state, then safely materializes that tree into the selected visible project root. The root must still match an accepted Hop ancestor, and ignored or untracked destination collisions block before acceptance. Materialization uses a disposable index and never moves HEAD, the active branch, or the user's real index. +`hop land` is the Desktop operation. It performs a real Git three-way content +merge, so compatible edits in the same file and identical changes compose +automatically. It validates and advances accepted state, then safely +materializes that tree into the selected visible project root. The root must +still match an accepted Hop ancestor, and ignored or untracked destination +collisions block before acceptance. Materialization uses a disposable index and +never moves HEAD, the active branch, or the user's real index. + +When the three-way merge has genuine unresolved conflicts, `hop land` returns +exit `20` and automatically prepares a reconciliation prompt in the original +task but a fresh isolated attempt/workspace. Its JSON includes +`reconciliation.prompt`, `workspace`, +`conflicts`, and the proposal/current accepted states. The agent adopts that +prompt/workspace, resolves both intents, checks, proposes, and lands again. +Structural, binary, delete/rename, mode, and symlink conflicts may have no text +markers, so the agent must inspect both returned input states. Hop requires a +successful `hop check` on the resolved tree before reproposal. The user is not +asked to coordinate ordinary code conflicts. `hop refresh` is the idempotent +explicit form of the same preparation step. `hop accept` is the controller/kernel operation. It advances SQLite and `refs/hop/accepted` but intentionally leaves the visible root untouched. @@ -105,7 +126,7 @@ command, summary, output, or source file. | `0` | Success | | `1` | Git, SQLite, filesystem, or internal error | | `2` | Invalid CLI usage | -| `20` | Overlap or conservative conflict block | +| `20` | Genuine three-way merge conflict; reconciliation workspace prepared | | `21` | Accepted or attempt head changed during compare-and-swap | | `22` | Validation command failed | | `23` | Visible project root diverged or contains an overwrite collision | @@ -146,7 +167,10 @@ provide the same boundary inside compatible agent clients. - **Check failure:** fix the live workspace, checkpoint/check again, then create a new proposal. - **Review-only request:** preserve and report the proposal without landing it. - **Frozen proposal needs changes:** record a follow-up prompt; never mutate the stored proposal. -- **Overlap on landing:** retain both lineages and reconcile through a new prompt against current accepted state. +- **Merge conflict on landing:** continue automatically in the returned + reconciliation prompt/workspace; inspect both inputs, resolve textual and + structural conflicts, validate, propose, and land again. Stop only for + product ambiguity, not ordinary textual overlap. - **Visible-root conflict:** preserve the proposal and the user's files. Do not substitute controller-only `hop accept`; resolve or capture the visible changes, then land again. - **Controller-accepted root is stale:** run `hop sync`; it succeeds only from an accepted ancestor and never overwrites divergence. - **Ref inconsistency:** run `hop doctor`; use `hop doctor --repair` only outside final validation.