From 2f00b2dd3f8bc5e4c261563a9ac063d1716f0d47 Mon Sep 17 00:00:00 2001 From: Hop Date: Sat, 11 Jul 2026 13:55:33 -0700 Subject: [PATCH] Finalize safe session rollover without nested lock deadlock Hop-State: A_06FN5XPBA1JMJNPS2MAR0D0 Hop-Proposal: R_06FN5XN2CX7XBZ4KF1JZZ2G Hop-Task: T_06FN3MBF98GWD4NA5PA1RWG Hop-Attempt: AT_06FN5TMC0J7DXV2JGF15T20 --- internal/hop/db.go | 138 ++++++++++++++++ internal/hop/service.go | 90 ++++++++++- internal/hop/service_smoke_test.go | 251 ++++++++++++++++++++++++++++- skills/hop/SKILL.md | 11 +- skills/hop/references/protocol.md | 10 +- wiki/Agent-Workflow.md | 7 +- wiki/Core-Concepts.md | 6 +- 7 files changed, 499 insertions(+), 14 deletions(-) diff --git a/internal/hop/db.go b/internal/hop/db.go index 767f83e..c19b46d 100644 --- a/internal/hop/db.go +++ b/internal/hop/db.go @@ -415,6 +415,41 @@ func (s *Store) SetAgentSessionHead(ctx context.Context, agent, sessionID, state return nil } +// RetargetAgentSessions moves interactive sessions that currently point into +// one attempt to a successor prompt. Reconciliation uses this so a follow-up +// received while conflicts are being resolved reaches the reconciliation +// workspace instead of reopening the stale source workspace. +func (s *Store) RetargetAgentSessions(ctx context.Context, agent, fromAttemptID, expectedHeadID, toStateID string) error { + if strings.TrimSpace(agent) == "" || strings.TrimSpace(fromAttemptID) == "" || + strings.TrimSpace(expectedHeadID) == "" || strings.TrimSpace(toStateID) == "" { + return errors.New("hop: agent, source attempt, expected head, and target state are required") + } + tx, err := s.db.BeginTx(ctx, nil) + if err != nil { + return fmt.Errorf("begin agent session retarget: %w", err) + } + defer tx.Rollback() + attempt, err := attemptTx(ctx, tx, fromAttemptID) + if err != nil { + return err + } + if attempt.HeadStateID != expectedHeadID { + return &HeadChangedError{Scope: "attempt", Expected: expectedHeadID, Actual: attempt.HeadStateID} + } + if _, err := tx.ExecContext(ctx, + `UPDATE agent_sessions + SET head_state_id = ?, updated_at = ? + WHERE agent = ? + AND head_state_id IN (SELECT id FROM states WHERE attempt_id = ?)`, + toStateID, formatTime(time.Now().UTC()), agent, fromAttemptID); err != nil { + return fmt.Errorf("retarget agent sessions: %w", err) + } + if err := tx.Commit(); err != nil { + return fmt.Errorf("commit agent session retarget: %w", err) + } + return nil +} + // CreateTaskAttemptPrompt atomically creates all three records. The prompt is // durable and the attempt head points to it before the transaction becomes // visible, so an orchestrator may safely deliver the prompt after this returns. @@ -567,6 +602,8 @@ func (s *Store) CreateAttemptPrompt( attempt Attempt, prompt State, parents []Parent, + sessionSourceAttemptID string, + expectedSourceHeadID string, ) (Attempt, State, error) { now := time.Now().UTC() if attempt.ID == "" { @@ -625,6 +662,20 @@ func (s *Store) CreateAttemptPrompt( 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) } + if sessionSourceAttemptID != "" { + if expectedSourceHeadID == "" { + return Attempt{}, State{}, errors.New("hop: reconciliation source head is required") + } + sourceAttempt, err := attemptTx(ctx, tx, sessionSourceAttemptID) + if err != nil { + return Attempt{}, State{}, err + } + if sourceAttempt.HeadStateID != expectedSourceHeadID { + return Attempt{}, State{}, &HeadChangedError{ + Scope: "attempt", Expected: expectedSourceHeadID, Actual: sourceAttempt.HeadStateID, + } + } + } base, err := stateTx(ctx, tx, attempt.BaseStateID) if err != nil { return Attempt{}, State{}, fmt.Errorf("read reconciliation base state: %w", err) @@ -659,6 +710,16 @@ func (s *Store) CreateAttemptPrompt( `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 sessionSourceAttemptID != "" { + if _, err := tx.ExecContext(ctx, + `UPDATE agent_sessions + SET head_state_id = ?, updated_at = ? + WHERE agent = ? + AND head_state_id IN (SELECT id FROM states WHERE attempt_id = ?)`, + prompt.ID, formatTime(now), attempt.Agent, sessionSourceAttemptID); err != nil { + return Attempt{}, State{}, fmt.Errorf("retarget reconciliation sessions: %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) @@ -839,6 +900,17 @@ func (s *Store) CASAccept( if err != nil { return State{}, fmt.Errorf("read proposal parent: %w", err) } + if proposal.AttemptID != "" { + attempt, err := attemptTx(ctx, tx, proposal.AttemptID) + if err != nil { + return State{}, err + } + if attempt.HeadStateID != proposal.ID { + return State{}, &HeadChangedError{ + Scope: "attempt", Expected: proposal.ID, Actual: attempt.HeadStateID, + } + } + } if accepted.TaskID == "" { accepted.TaskID = proposal.TaskID } @@ -886,6 +958,17 @@ func (s *Store) CASAccept( } } if accepted.TaskID != "" { + if accepted.AttemptID != "" { + if _, err := tx.ExecContext(ctx, + `UPDATE attempts + SET status = 'completed' + WHERE task_id = ? + AND id != ? + AND status NOT IN ('accepted', 'completed', 'failed', 'cancelled', 'rejected')`, + accepted.TaskID, accepted.AttemptID); err != nil { + return State{}, fmt.Errorf("complete superseded task attempts: %w", err) + } + } if _, err := tx.ExecContext(ctx, `UPDATE tasks SET status = 'accepted' WHERE id = ?`, accepted.TaskID); err != nil { return State{}, fmt.Errorf("mark task accepted: %w", err) @@ -949,6 +1032,33 @@ func (s *Store) AcceptedForProposal(ctx context.Context, proposalID string) (Sta return state, true, nil } +// AcceptedForTask returns the latest immutable accepted outcome produced by a +// task. Unlike the mutable task status, this remains authoritative when an +// older client has accidentally reactivated an already accepted task. +func (s *Store) AcceptedForTask(ctx context.Context, taskID string) (State, bool, error) { + if strings.TrimSpace(taskID) == "" { + return State{}, false, errors.New("hop: task ID is required") + } + var id string + err := s.db.QueryRowContext(ctx, + `SELECT id + FROM states + WHERE task_id = ? AND kind = 'accepted' + ORDER BY created_at DESC, id DESC + LIMIT 1`, taskID).Scan(&id) + if errors.Is(err, sql.ErrNoRows) { + return State{}, false, nil + } + if err != nil { + return State{}, false, fmt.Errorf("find accepted state for task %s: %w", taskID, err) + } + state, err := s.GetState(ctx, id) + if err != nil { + return State{}, false, err + } + 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. @@ -1105,6 +1215,34 @@ func (s *Store) GetParents(ctx context.Context, stateID string) ([]Parent, error return parents, nil } +// StateDescendsFrom reports whether descendantID is the same state as, or has +// any typed-parent path to, ancestorID. Session rollover uses state ancestry so +// a mutable task status cannot hide unfinished prompts created after an older +// accepted outcome. +func (s *Store) StateDescendsFrom(ctx context.Context, descendantID, ancestorID string) (bool, error) { + if strings.TrimSpace(descendantID) == "" || strings.TrimSpace(ancestorID) == "" { + return false, errors.New("hop: descendant and ancestor state IDs are required") + } + var found int + err := s.db.QueryRowContext(ctx, + `WITH RECURSIVE ancestry(id) AS ( + SELECT ? + UNION + SELECT parents.parent_state_id + FROM state_parents parents + JOIN ancestry ON parents.state_id = ancestry.id + ) + SELECT 1 FROM ancestry WHERE id = ? LIMIT 1`, + descendantID, ancestorID).Scan(&found) + if errors.Is(err, sql.ErrNoRows) { + return false, nil + } + if err != nil { + return false, fmt.Errorf("inspect state ancestry: %w", err) + } + return found == 1, nil +} + func (s *Store) ParentByRole(ctx context.Context, stateID, role string) (Parent, error) { var parent Parent err := s.db.QueryRowContext(ctx, diff --git a/internal/hop/service.go b/internal/hop/service.go index 73a7acb..b339a86 100644 --- a/internal/hop/service.go +++ b/internal/hop/service.go @@ -194,6 +194,9 @@ func (s *Service) BeginPrompt(ctx context.Context, message, fromStateID, agent, if redactedSession, findings := RedactPromptSecrets(sessionID); len(findings) > 0 || redactedSession != sessionID { return PromptResult{}, errors.New("hop: refusing to use a potential credential as an agent session ID") } + // Serialize interactive prompt capture with other begins. Acceptance checks + // the proposal's attempt head again inside its SQLite transaction, so a begin + // racing a land makes that proposal stale without sharing accept.lock. release, err := acquireProjectLock(ctx, s.Root, "prompt") if err != nil { return PromptResult{}, err @@ -204,7 +207,13 @@ func (s *Service) BeginPrompt(ctx context.Context, message, fromStateID, agent, if stateID, exists, err := s.Store.AgentSessionHead(ctx, agent, sessionID); err != nil { return PromptResult{}, err } else if exists { - fromStateID = stateID + continuable, err := s.sessionStateContinuable(ctx, stateID) + if err != nil { + return PromptResult{}, err + } + if continuable { + fromStateID = stateID + } } } result, err := s.CreatePrompt(ctx, message, fromStateID, agent) @@ -219,6 +228,68 @@ func (s *Service) BeginPrompt(ctx context.Context, message, fromStateID, agent, return result, nil } +// sessionStateContinuable decides whether an implicit interactive-session +// pointer still represents unfinished work. An immutable accepted state for +// the task ends ordinary session continuation even if an older Hop build later +// changed the task's mutable status back to active. A dirty post-proposal +// workspace is preserved by continuing it instead of silently abandoning work. +func (s *Service) sessionStateContinuable(ctx context.Context, stateID string) (bool, error) { + state, err := s.Store.GetState(ctx, stateID) + if err != nil { + return false, err + } + if state.TaskID == "" || state.AttemptID == "" { + return false, fmt.Errorf("session state %s does not belong to a task attempt", state.ID) + } + accepted, exists, err := s.Store.AcceptedForTask(ctx, state.TaskID) + if err != nil { + return false, err + } + if !exists { + return true, nil + } + attempt, err := s.Store.GetAttempt(ctx, state.AttemptID) + if err != nil { + return false, err + } + head, err := s.Store.GetState(ctx, attempt.HeadStateID) + if err != nil { + return false, err + } + covered, err := s.Store.StateDescendsFrom(ctx, accepted.ID, head.ID) + if err != nil { + return false, err + } + if !covered { + return true, nil + } + workspaceRepo, err := OpenRepository(attempt.Workspace) + if err != nil { + return false, err + } + workspaceTree, err := workspaceRepo.WorktreeTree(ctx, head.SourceTree) + if err != nil { + return false, err + } + if workspaceTree != head.SourceTree { + return true, nil + } + + status := "completed" + if attempt.ID == accepted.AttemptID { + status = "accepted" + } + if attempt.Status != status { + if err := s.Store.UpdateAttemptStatus(ctx, attempt.ID, status); err != nil { + return false, err + } + } + if err := s.Store.UpdateTaskStatus(ctx, state.TaskID, "accepted"); err != nil { + return false, err + } + return false, nil +} + func (s *Service) createInitialPrompt(ctx context.Context, message, agent string) (PromptResult, error) { accepted, err := s.Store.AcceptedHead(ctx) if err != nil { @@ -603,6 +674,11 @@ func (s *Service) accept(ctx context.Context, proposalID string, checkCommand [] if err != nil { return AcceptResult{}, err } + if attempt.HeadStateID != proposal.ID { + return AcceptResult{}, &HeadChangedError{ + Scope: "attempt", Expected: proposal.ID, Actual: attempt.HeadStateID, + } + } baseStateID := proposal.CanonicalAnchorID if baseStateID == "" { baseStateID = attempt.BaseStateID @@ -804,6 +880,11 @@ func (s *Service) Refresh(ctx context.Context, proposalID string) (RefreshResult if err != nil { return RefreshResult{}, err } + if sourceAttempt.HeadStateID != proposal.ID { + return RefreshResult{}, &HeadChangedError{ + Scope: "attempt", Expected: proposal.ID, Actual: sourceAttempt.HeadStateID, + } + } task, err := s.Store.GetTask(ctx, proposal.TaskID) if err != nil { return RefreshResult{}, err @@ -815,6 +896,9 @@ func (s *Service) Refresh(ctx context.Context, proposalID string) (RefreshResult if existing, exists, err := s.Store.ReconciliationPrompt(ctx, proposal.ID, current.ID); err != nil { return RefreshResult{}, err } else if exists { + if err := s.Store.RetargetAgentSessions(ctx, sourceAttempt.Agent, sourceAttempt.ID, proposal.ID, existing.ID); err != nil { + return RefreshResult{}, err + } return s.reconciliationResult(ctx, existing, task, proposal, current, true) } baseStateID := proposal.CanonicalAnchorID @@ -901,7 +985,9 @@ func (s *Service) Refresh(ctx context.Context, proposalID string) (RefreshResult _ = s.Repo.RemoveWorktree(context.Background(), workspace, true) } }() - reconciliationAttempt, prompt, err = s.Store.CreateAttemptPrompt(ctx, reconciliationAttempt, prompt, parents) + reconciliationAttempt, prompt, err = s.Store.CreateAttemptPrompt( + ctx, reconciliationAttempt, prompt, parents, sourceAttempt.ID, proposal.ID, + ) if err != nil { return RefreshResult{}, err } diff --git a/internal/hop/service_smoke_test.go b/internal/hop/service_smoke_test.go index cb0c3dc..5644439 100644 --- a/internal/hop/service_smoke_test.go +++ b/internal/hop/service_smoke_test.go @@ -242,7 +242,7 @@ func TestOpenProjectWaitsForInitializationLock(t *testing.T) { if err != nil { t.Fatal(err) } - case <-time.After(5 * time.Second): + case <-time.After(10 * time.Second): t.Fatal("OpenProject did not resume after initialization lock release") } } @@ -364,6 +364,255 @@ func TestCLIBeginAutoInitializesAndContinuesCodexSession(t *testing.T) { }) } +func TestBeginRollsAcceptedReconciliationSessionToCurrentHead(t *testing.T) { + ctx := context.Background() + service, _ := newTestProject(t, map[string]string{"shared.txt": "color=base\n"}) + + session, err := service.BeginPrompt(ctx, "Use blue", "", "codex", "thread-rollover") + if err != nil { + t.Fatal(err) + } + concurrent, err := service.CreatePrompt(ctx, "Use red", "", "other") + if err != nil { + t.Fatal(err) + } + writeTestFile(t, filepath.Join(session.Workspace, "shared.txt"), "color=blue\n") + writeTestFile(t, filepath.Join(concurrent.Workspace, "shared.txt"), "color=red\n") + + sessionProposal, err := service.Propose(ctx, session.Prompt.ID, "Blue") + if err != nil { + t.Fatal(err) + } + concurrentProposal, err := service.Propose(ctx, concurrent.Prompt.ID, "Red") + if err != nil { + t.Fatal(err) + } + if _, err := service.Land(ctx, concurrentProposal.Proposal.ID, nil); err != nil { + t.Fatal(err) + } + if _, err := service.Land(ctx, sessionProposal.Proposal.ID, nil); err == nil { + t.Fatal("stale session proposal unexpectedly landed without reconciliation") + } else { + var conflict *ConflictError + if !errors.As(err, &conflict) { + t.Fatalf("land error = %v, want ConflictError", err) + } + } + + reconciliation, err := service.Refresh(ctx, sessionProposal.Proposal.ID) + if err != nil { + t.Fatal(err) + } + reconciliationHead, exists, err := service.Store.AgentSessionHead(ctx, "codex", "thread-rollover") + if err != nil { + t.Fatal(err) + } + if !exists || reconciliationHead != reconciliation.Prompt.ID { + t.Fatalf("session head = %q, %v; want reconciliation prompt %s", reconciliationHead, exists, reconciliation.Prompt.ID) + } + writeTestFile(t, filepath.Join(reconciliation.Workspace, "shared.txt"), "color=red-blue\n") + if _, err := service.RunCheck(ctx, reconciliation.Prompt.ID, []string{ + "sh", "-c", `test "$(cat shared.txt)" = "color=red-blue"`, + }); err != nil { + t.Fatal(err) + } + resolvedProposal, err := service.Propose(ctx, reconciliation.Prompt.ID, "Resolve red and blue") + if err != nil { + t.Fatal(err) + } + if _, err := service.Land(ctx, resolvedProposal.Proposal.ID, nil); err != nil { + t.Fatal(err) + } + sourceAttempt, err := service.Store.GetAttempt(ctx, session.Attempt.ID) + if err != nil { + t.Fatal(err) + } + if sourceAttempt.Status != "completed" { + t.Fatalf("source attempt status = %q, want completed", sourceAttempt.Status) + } + + // Rollover must use the latest global accepted state, not merely this task's + // accepted outcome. + later, err := service.CreatePrompt(ctx, "Add a later accepted change", "", "other") + if err != nil { + t.Fatal(err) + } + writeTestFile(t, filepath.Join(later.Workspace, "later.txt"), "later\n") + laterProposal, err := service.Propose(ctx, later.Prompt.ID, "Later accepted change") + if err != nil { + t.Fatal(err) + } + laterAccepted, err := service.Land(ctx, laterProposal.Proposal.ID, nil) + if err != nil { + t.Fatal(err) + } + + next, err := service.BeginPrompt(ctx, "Next request", "", "codex", "thread-rollover") + if err != nil { + t.Fatal(err) + } + assertFreshRollover := func(label string, result PromptResult) { + t.Helper() + if result.Checkpoint != nil { + t.Fatalf("%s continued accepted session with checkpoint %s", label, result.Checkpoint.ID) + } + if result.Attempt.ID == session.Attempt.ID || result.Attempt.ID == reconciliation.Attempt.ID { + t.Fatalf("%s reused completed attempt %s", label, result.Attempt.ID) + } + if result.Task.ID == session.Task.ID { + t.Fatalf("%s reopened accepted task %s", label, result.Task.ID) + } + if result.Attempt.BaseStateID != laterAccepted.State.ID { + t.Fatalf("%s base = %s, want %s", label, result.Attempt.BaseStateID, laterAccepted.State.ID) + } + if result.Prompt.CanonicalAnchorID != laterAccepted.State.ID || + result.Prompt.SourceTree != laterAccepted.State.SourceTree || + result.Prompt.GitCommit != laterAccepted.State.GitCommit { + t.Fatalf("%s prompt was not rooted at latest accepted head: %#v; accepted=%#v", label, result.Prompt, laterAccepted.State) + } + if contents, err := os.ReadFile(filepath.Join(result.Workspace, "shared.txt")); err != nil || string(contents) != "color=red-blue\n" { + t.Fatalf("%s shared workspace = %q, err=%v", label, string(contents), err) + } + if contents, err := os.ReadFile(filepath.Join(result.Workspace, "later.txt")); err != nil || string(contents) != "later\n" { + t.Fatalf("%s later workspace = %q, err=%v", label, string(contents), err) + } + } + assertFreshRollover("normal reconciliation session", next) + head, exists, err := service.Store.AgentSessionHead(ctx, "codex", "thread-rollover") + if err != nil { + t.Fatal(err) + } + if !exists || head != next.Prompt.ID { + t.Fatalf("session head = %q, %v; want %s", head, exists, next.Prompt.ID) + } + + // Older Hop builds could reactivate both records after acceptance. The + // immutable accepted outcome, not these mutable statuses, must drive rollover. + if err := service.Store.UpdateAttemptStatus(ctx, session.Attempt.ID, "active"); err != nil { + t.Fatal(err) + } + if err := service.Store.UpdateTaskStatus(ctx, session.Task.ID, "active"); err != nil { + t.Fatal(err) + } + if err := service.Store.SetAgentSessionHead(ctx, "codex", "legacy-thread-rollover", session.Prompt.ID); err != nil { + t.Fatal(err) + } + legacyNext, err := service.BeginPrompt(ctx, "Legacy next request", "", "codex", "legacy-thread-rollover") + if err != nil { + t.Fatal(err) + } + assertFreshRollover("legacy session", legacyNext) + sourceAttempt, err = service.Store.GetAttempt(ctx, session.Attempt.ID) + if err != nil { + t.Fatal(err) + } + if sourceAttempt.Status == "active" { + t.Fatalf("source attempt %s was left active", sourceAttempt.ID) + } + head, exists, err = service.Store.AgentSessionHead(ctx, "codex", "legacy-thread-rollover") + if err != nil { + t.Fatal(err) + } + if !exists || head != legacyNext.Prompt.ID { + t.Fatalf("legacy session head = %q, %v; want %s", head, exists, legacyNext.Prompt.ID) + } +} + +func TestLandRejectsProposalSupersededByFollowup(t *testing.T) { + ctx := context.Background() + service, initial := newTestProject(t, map[string]string{"base.txt": "base\n"}) + started, err := service.BeginPrompt(ctx, "Create a change", "", "codex", "thread-stale-proposal") + if err != nil { + t.Fatal(err) + } + writeTestFile(t, filepath.Join(started.Workspace, "change.txt"), "first\n") + proposal, err := service.Propose(ctx, started.Prompt.ID, "First change") + if err != nil { + t.Fatal(err) + } + followup, err := service.BeginPrompt(ctx, "Refine that change", "", "codex", "thread-stale-proposal") + if err != nil { + t.Fatal(err) + } + if followup.Attempt.ID != started.Attempt.ID || followup.Checkpoint == nil { + t.Fatalf("follow-up did not advance the original attempt: %#v", followup) + } + if _, err := service.Land(ctx, proposal.Proposal.ID, nil); !errors.Is(err, ErrAttemptHeadChanged) { + t.Fatalf("superseded proposal land error = %v, want ErrAttemptHeadChanged", err) + } + parents := canonicalizeParents([]Parent{ + {StateID: initial.ID, Role: "canonical_parent", Order: 0}, + {StateID: proposal.Proposal.ID, Role: "proposal_parent", Order: 1}, + }) + accepted := State{ + ID: newID("a"), + Kind: StateAccepted, + TaskID: proposal.Proposal.TaskID, + AttemptID: proposal.Proposal.AttemptID, + CanonicalAnchorID: initial.ID, + SourceTree: proposal.Proposal.SourceTree, + GitCommit: proposal.Proposal.GitCommit, + Summary: proposal.Proposal.Summary, + Agent: proposal.Proposal.Agent, + CreatedAt: time.Now().UTC(), + Parents: parents, + } + accepted.Digest, err = digestState(accepted, parents) + if err != nil { + t.Fatal(err) + } + if _, err := service.Store.CASAccept(ctx, initial.ID, accepted, parents); !errors.Is(err, ErrAttemptHeadChanged) { + t.Fatalf("transactional stale proposal error = %v, want ErrAttemptHeadChanged", err) + } + head, err := service.Store.AcceptedHead(ctx) + if err != nil { + t.Fatal(err) + } + if head.ID != initial.ID { + t.Fatalf("superseded proposal advanced accepted head to %s", head.ID) + } +} + +func TestBeginKeepsUnfinishedStateCreatedAfterAcceptedOutcome(t *testing.T) { + ctx := context.Background() + service, _ := newTestProject(t, map[string]string{"base.txt": "base\n"}) + started, err := service.BeginPrompt(ctx, "Land the first change", "", "codex", "thread-post-accept") + if err != nil { + t.Fatal(err) + } + writeTestFile(t, filepath.Join(started.Workspace, "landed.txt"), "landed\n") + proposal, err := service.Propose(ctx, started.Prompt.ID, "Landed change") + if err != nil { + t.Fatal(err) + } + if _, err := service.Land(ctx, proposal.Proposal.ID, nil); err != nil { + t.Fatal(err) + } + + // Although agents must not mutate a frozen proposal, preserve any such + // residual work rather than silently rolling it away. + writeTestFile(t, filepath.Join(started.Workspace, "pending.txt"), "pending\n") + continued, err := service.BeginPrompt(ctx, "Preserve the pending work", "", "codex", "thread-post-accept") + if err != nil { + t.Fatal(err) + } + if continued.Attempt.ID != started.Attempt.ID || continued.Checkpoint == nil { + t.Fatalf("dirty accepted workspace was not preserved: %#v", continued) + } + assertTreeFiles(t, service, continued.Checkpoint.GitCommit, map[string]string{ + "base.txt": "base\n", + "landed.txt": "landed\n", + "pending.txt": "pending\n", + }) + again, err := service.BeginPrompt(ctx, "Continue that pending work", "", "codex", "thread-post-accept") + if err != nil { + t.Fatal(err) + } + if again.Attempt.ID != started.Attempt.ID || again.Checkpoint == nil { + t.Fatalf("post-acceptance prompt lineage was abandoned: %#v", again) + } +} + func TestDoctorRejectsCommitTreeAndDigestMismatch(t *testing.T) { ctx := context.Background() service, initial := newTestProject(t, map[string]string{"base.txt": "base\n"}) diff --git a/skills/hop/SKILL.md b/skills/hop/SKILL.md index 8ef238b..590b2d5 100644 --- a/skills/hop/SKILL.md +++ b/skills/hop/SKILL.md @@ -29,9 +29,11 @@ only the shell-added final newline. Never copy the credential anywhere else. `hop begin` performs the Desktop bootstrap: - Initialize Hop automatically when the project has not used it before. -- Use `CODEX_THREAD_ID` to bind this Codex task to one Hop attempt. +- Use `CODEX_THREAD_ID` to bind this Codex task to its unfinished Hop work. - Create a prompt state and isolated workspace on the first turn. -- Checkpoint prior workspace effects and append a prompt state on follow-ups. +- Checkpoint prior workspace effects and append follow-ups until that work lands. +- Follow a reconciliation into its fresh attempt, then start the first prompt + after landing from the latest accepted state instead of reopening old work. - Redact detected API keys, tokens, passwords, private keys, authorization headers, and credential-bearing connection strings before persistence. @@ -114,8 +116,9 @@ For a read-only or informational turn, the prompt state is sufficient; do not invent a proposal when the workspace tree is unchanged. Do not edit a frozen proposal. A user follow-up triggers this skill again; -run `hop begin` again before acting. Session binding selects the existing -attempt automatically, so the user never needs to carry state IDs. +run `hop begin` again before acting. Session binding selects unfinished work +automatically and rolls completed work onto the latest accepted state, so the +user never needs to carry state IDs. ## Auto-accept by default diff --git a/skills/hop/references/protocol.md b/skills/hop/references/protocol.md index 426af4f..34d2481 100644 --- a/skills/hop/references/protocol.md +++ b/skills/hop/references/protocol.md @@ -37,7 +37,9 @@ Prompt, checkpoint, and proposal states may reference identical Git trees while Interactive agents may begin without these variables. `hop begin` returns the equivalent IDs and workspace, while `CODEX_THREAD_ID` binds later messages in -the same Codex task to the existing attempt. +the same Codex task to unfinished work. Follow-ups before acceptance continue +the attempt; the first prompt after acceptance starts a fresh task and attempt +at the latest accepted state. ## Command contract @@ -111,8 +113,10 @@ accepted state, including projects created with older Hop builds. `hop begin` is the Codex Desktop entry point. It initializes Hop when necessary, captures the current message before the agent performs project work, and uses `CODEX_THREAD_ID` as the default session key. A later `hop begin` in the same -Codex task checkpoints the prior workspace before appending the follow-up -prompt state. +Codex task checkpoints the prior workspace before appending a follow-up while +that work remains unfinished. Reconciliation transfers the session to its fresh +attempt. After a proposal is accepted, the next `hop begin` starts from the +latest accepted state and never reopens the completed workspace. Pass the original message to `hop begin` without model-side redaction. Hop's sanitizer replaces detected credential values before any durable write and diff --git a/wiki/Agent-Workflow.md b/wiki/Agent-Workflow.md index 8bff1a9..ac81245 100644 --- a/wiki/Agent-Workflow.md +++ b/wiki/Agent-Workflow.md @@ -35,8 +35,11 @@ capture boundary. ## Follow-up messages A later `hop begin` with the same Codex task session checkpoints existing -workspace effects, appends a new prompt state, and continues the same attempt. -The user does not carry state IDs between messages. +workspace effects, appends a new prompt state, and continues the same attempt +while that work remains unfinished. If Hop prepares reconciliation, the session +follows its fresh workspace. After the result lands, the next prompt starts a +new task and attempt rooted at the latest accepted state. Completed workspaces +are never reopened, and the user does not carry state IDs between messages. ## Controller-grade capture diff --git a/wiki/Core-Concepts.md b/wiki/Core-Concepts.md index 756e3bb..77fe933 100644 --- a/wiki/Core-Concepts.md +++ b/wiki/Core-Concepts.md @@ -21,8 +21,10 @@ different moments and causal roles. ## Task A task groups the prompts and attempts pursuing one user outcome. Follow-up -messages in the same Codex task stay connected automatically through -`CODEX_THREAD_ID`. +messages pursuing unfinished work stay connected automatically through +`CODEX_THREAD_ID`. Once that outcome is accepted, the next message starts a new +Hop task at the latest accepted state even when the Codex conversation stays +open. ## Attempt and workspace