fix(review): reserve answer time and retain failure evidence

docs/changes/unreleased/1817-review-answer-deadline.md

@@ -0,0 +1,12 @@
+---
+kind: fixed
+title: review agents reserve answer time and retain failure evidence
+pr: 1817
+surface: [chat, engine, docs]
+invalidates:
+  - "Review sessions were forced to answer only at their turn or context limits. They now reserve answer time inside the existing hard deadline."
+  - "Session duration was lost when the result returned. Completed and failed sessions now retain elapsed time and failed-agent diagnostics."
+  - "A pipeline error returned without saving a review record. It now saves separate non-postable failure records; the failed pipeline ending is preserved."
+---
+
+Failure records contain no assembled review and cannot be posted as GitHub reviews. Time and cost ceilings are unchanged.

internal/agentsession/app.go

@@ -66,6 +66,9 @@ type Config struct {
 type Limits struct {
 	Turns int
 	Wall  time.Duration
+	// AnswerReserve is part of Wall, not extra time. Zero preserves the
+	// program's existing reading policy.
+	AnswerReserve time.Duration
 }
 
 // limitsFor is the bounds one session runs under: the program's for this
@@ -166,7 +169,7 @@ func (a *App) Harness(ctx context.Context, prompt string, schema map[string]any,
 	limits := a.limitsFor(opts)
 	result, err := RunSession(ctx, a.client, SessionOrder{
 		Model: a.config.SessionModel, Thread: thread, Root: root, Work: a.config.Work, Prompt: prompt, Schema: schema,
-		MaxTurns: limits.Turns, Wall: limits.Wall, Policy: a.config.Policy,
+		MaxTurns: limits.Turns, Wall: limits.Wall, AnswerReserve: limits.AnswerReserve, Policy: a.config.Policy,
 	})
 	a.spend(result.CostUSD, true)
 	if watch := a.config.Watch; watch != nil && watch.Session != nil {

internal/agentsession/session.go

@@ -42,6 +42,9 @@ type SessionOrder struct {
 	MaxTurns int
 	// Wall is the session's own time bound; zero is none past the context's.
 	Wall time.Duration
+	// AnswerReserve stops reading this long before the existing deadline, so
+	// the session can answer from completed observations. Zero reserves none.
+	AnswerReserve time.Duration
 	// Policy is how the session reads and answers; every field is required.
 	Policy Policy
 }
@@ -68,12 +71,37 @@ var errNoAnswer = errors.New("the session ended without an answer")
 // could not run at all — the model API refused, the run was stopped; a session
 // that ran and gave no usable answer says so in Failed.
 func RunSession(ctx context.Context, client *Client, order SessionOrder) (SessionResult, error) {
-	started := time.Now()
+	return runSession(ctx, client.Complete, order, sessionClock{time.Now, context.WithDeadline})
+}
+
+// sessionClock keeps the deadline transition testable without waiting out a
+// real session. Production always uses the context's actual deadline.
+type sessionClock struct {
+	now          func() time.Time
+	withDeadline func(context.Context, time.Time) (context.Context, context.CancelFunc)
+}
+
+func runSession(ctx context.Context, complete func(context.Context, Request) (*ai.Response, error), order SessionOrder, clock sessionClock) (result SessionResult, err error) {
+	started := clock.now()
+	defer func() { result.Duration = clock.now().Sub(started) }()
 	if order.Wall > 0 {
 		var cancel context.CancelFunc
-		ctx, cancel = context.WithTimeout(ctx, order.Wall)
+		ctx, cancel = clock.withDeadline(ctx, started.Add(order.Wall))
 		defer cancel()
 	}
+	readingCtx := ctx
+	var readingDeadline time.Time
+	if deadline, ok := ctx.Deadline(); ok && order.AnswerReserve > 0 {
+		// A caller's shorter deadline wins. Never reserve more than half the
+		// available time, and never extend either the caller or session wall.
+		reserve := min(order.AnswerReserve, deadline.Sub(started)/2)
+		if reserve > 0 {
+			readingDeadline = deadline.Add(-reserve)
+			var cancel context.CancelFunc
+			readingCtx, cancel = clock.withDeadline(ctx, readingDeadline)
+			defer cancel()
+		}
+	}
 	maxTurns := order.MaxTurns
 	if maxTurns <= 0 {
 		return SessionResult{}, unsetError("SessionOrder", "MaxTurns")
@@ -95,27 +123,38 @@ func RunSession(ctx context.Context, client *Client, order SessionOrder) (Sessio
 		textMessage("system", sessionSystem(order)),
 		textMessage("user", order.Prompt),
 	}
-	result := SessionResult{}
-	defer func() { result.Duration = time.Since(started) }()
 	size := len(order.Prompt)
 	asked := 0
 	answering := false
 	for result.Turns < maxTurns+policy.FollowUps+1 {
 		// THE LAST TURN, OR A FULL CONTEXT, IS AN ANSWER. A session that is
 		// still reading when its turns or its room run out is told to answer
 		// with what it has, and is offered no tools to do anything else.
-		if !answering && (result.Turns >= maxTurns-1 || size > policy.ContextChars) {
+		if !answering && (result.Turns >= maxTurns-1 || size > policy.ContextChars ||
+			(!readingDeadline.IsZero() && !clock.now().Before(readingDeadline))) {
 			answering = true
 			messages = append(messages, textMessage("user", policy.AnswerNow))
 		}
 		request := Request{Model: order.Model, Thread: order.Thread, Messages: messages}
 		if !answering {
 			request.Tools = tools.definitions()
 		}
-		response, err := client.Complete(ctx, request)
+		callCtx := ctx
+		if !answering {
+			callCtx = readingCtx
+		}
+		response, err := complete(callCtx, request)
 		result.Turns++
 		result.CostUSD += costOf(response)
 		if err != nil {
+			// Only our intentional reading cutoff starts an answer. A caller
+			// stop, provider refusal or exhausted full deadline still fails.
+			if !answering && !readingDeadline.IsZero() && errors.Is(readingCtx.Err(), context.DeadlineExceeded) &&
+				ctx.Err() == nil && (errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled)) {
+				answering = true
+				messages = append(messages, textMessage("user", policy.AnswerNow))
+				continue
+			}
 			result.Failed = err.Error()
 			return result, err
 		}

internal/agentsession/session_deadline_test.go

@@ -0,0 +1,201 @@
+package agentsession
+
+import (
+	"context"
+	"errors"
+	"testing"
+	"time"
+
+	"github.com/Agent-Field/agentfield/sdk/go/ai"
+)
+
+func TestSessionDurationIncludesTheCompletedAnswer(t *testing.T) {
+	api := (&fakeAPI{}).then(answer("done", 0))
+	result, err := RunSession(t.Context(), api.serve(t), SessionOrder{
+		Root: repo(t), Prompt: "answer", MaxTurns: 6, Policy: testPolicy,
+	})
+	if err != nil || result.Text != "done" {
+		t.Fatalf("answer = %+v, error = %v", result, err)
+	}
+	if result.Duration <= 0 {
+		t.Fatal("the completed session has no elapsed time")
+	}
+}
+
+// A manual clock expires contexts at their recorded deadlines. Advancing an
+// in-flight read reproduces the provider cancellation without real sleeps.
+type sessionTestClock struct {
+	now      time.Time
+	contexts []*sessionTestContext
+}
+
+type sessionTestContext struct {
+	context.Context
+	parent   context.Context
+	deadline time.Time
+	clock    *sessionTestClock
+	cancel   context.CancelFunc
+}
+
+func (c *sessionTestContext) Deadline() (time.Time, bool) { return c.deadline, true }
+func (c *sessionTestContext) Err() error {
+	if err := c.parent.Err(); err != nil {
+		return err
+	}
+	if !c.clock.now.Before(c.deadline) {
+		return context.DeadlineExceeded
+	}
+	return c.Context.Err()
+}
+
+func (c *sessionTestClock) runtime() sessionClock {
+	return sessionClock{now: func() time.Time { return c.now }, withDeadline: func(parent context.Context, deadline time.Time) (context.Context, context.CancelFunc) {
+		if bound, ok := parent.Deadline(); ok && bound.Before(deadline) {
+			deadline = bound
+		}
+		ctx, cancel := context.WithCancel(parent)
+		out := &sessionTestContext{Context: ctx, parent: parent, deadline: deadline, clock: c, cancel: cancel}
+		c.contexts = append(c.contexts, out)
+		return out, cancel
+	}}
+}
+
+func (c *sessionTestClock) advance(to time.Time) {
+	c.now = to
+	for _, ctx := range c.contexts {
+		if !to.Before(ctx.deadline) {
+			ctx.cancel()
+		}
+	}
+}
+
+func TestSessionReadingCutoffLeavesTheAnswerInsideTheOriginalWall(t *testing.T) {
+	for _, reserve := range []time.Duration{0, 22500 * time.Millisecond} {
+		t.Run(reserve.String(), func(t *testing.T) {
+			start := time.Date(2099, 1, 1, 0, 0, 0, 0, time.UTC)
+			clock := &sessionTestClock{now: start}
+			calls := 0
+			complete := func(ctx context.Context, request Request) (*ai.Response, error) {
+				calls++
+				deadline, _ := ctx.Deadline()
+				if calls == 1 {
+					clock.advance(start.Add(10 * time.Second))
+					_, response := callTool("read_file", `{"path":"README.md"}`)(wireRequest{})
+					out := response.(ai.Response)
+					return &out, nil
+				}
+				if calls == 2 {
+					end := start.Add(28 * time.Second)
+					if deadline.Before(end) {
+						clock.advance(deadline)
+						return nil, ctx.Err()
+					}
+					clock.advance(end)
+					_, response := callTool("list_dir", `{"path":"."}`)(wireRequest{})
+					out := response.(ai.Response)
+					return &out, nil
+				}
+				if reserve > 0 && (len(request.Tools) != 0 || request.Messages[len(request.Messages)-1].Content[0].Text != testPolicy.AnswerNow) {
+					t.Fatal("the reading cutoff did not force an answer without tools")
+				}
+				if deadline != start.Add(45*time.Second) {
+					t.Fatalf("the answer's hard deadline changed: %v", deadline.Sub(start))
+				}
+				end := clock.now.Add(18 * time.Second)
+				if !end.Before(deadline) {
+					clock.advance(deadline)
+					return nil, ctx.Err()
+				}
+				clock.advance(end)
+				_, response := answer(`{"file":"README.md","line":1}`, 0)(wireRequest{})
+				out := response.(ai.Response)
+				return &out, nil
+			}
+			result, err := runSession(t.Context(), complete, SessionOrder{Root: repo(t), Prompt: "read then answer",
+				MaxTurns: 6, Wall: 45 * time.Second, AnswerReserve: reserve, Policy: testPolicy, Schema: findingSchema}, clock.runtime())
+			if reserve == 0 {
+				if !errors.Is(err, context.DeadlineExceeded) || result.Failed == "" || result.Duration != 45*time.Second {
+					t.Fatalf("the unreserved session should hit its wall: %+v, %v", result, err)
+				}
+			} else if err != nil || result.Failed != "" || len(result.JSON) == 0 || result.Tools != 1 || result.Duration != 40500*time.Millisecond {
+				t.Fatalf("the reserved session should answer from its completed read: %+v, %v", result, err)
+			}
+		})
+	}
+}
+
+func TestSessionAnswerReserveNeverHidesAStopOrExtendsADeadline(t *testing.T) {
+	for _, kind := range []string{"caller stop", "provider refusal", "provider timeout", "answer timeout"} {
+		t.Run(kind, func(t *testing.T) {
+			start := time.Date(2099, 1, 1, 0, 0, 0, 0, time.UTC)
+			clock := &sessionTestClock{now: start}
+			parent, stop := context.WithCancel(t.Context())
+			defer stop()
+			parent, cancel := clock.runtime().withDeadline(parent, start.Add(20*time.Second))
+			defer cancel()
+			calls := 0
+			failure := errors.New("provider timeout")
+			if kind == "provider refusal" {
+				failure = &RefusedError{Code: 403, Message: "refused"}
+			}
+			complete := func(ctx context.Context, request Request) (*ai.Response, error) {
+				calls++
+				deadline, _ := ctx.Deadline()
+				if calls == 1 {
+					if deadline != start.Add(10*time.Second) {
+						t.Fatal("reserve did not honor half the shorter caller deadline")
+					}
+					if kind == "caller stop" {
+						stop()
+						return nil, ctx.Err()
+					}
+					if kind != "answer timeout" {
+						return nil, failure
+					}
+					clock.advance(deadline)
+					return nil, ctx.Err()
+				}
+				if deadline != start.Add(20*time.Second) || len(request.Tools) != 0 {
+					t.Fatal("answer extended the caller deadline or kept reading")
+				}
+				clock.advance(deadline)
+				return nil, ctx.Err()
+			}
+			result, err := runSession(parent, complete, SessionOrder{Root: repo(t), MaxTurns: 6,
+				Wall: 45 * time.Second, AnswerReserve: 30 * time.Second, Policy: testPolicy}, clock.runtime())
+			wantCalls := 1
+			if kind == "answer timeout" {
+				wantCalls = 2
+			}
+			if err == nil || result.Failed == "" || calls != wantCalls {
+				t.Fatalf("failure was hidden: %+v, %v, %d calls", result, err, calls)
+			}
+		})
+	}
+}
+
+func TestSessionAtTheTimeCutoffStillChecksItsSchemaAndBoundsCorrections(t *testing.T) {
+	start := time.Date(2099, 1, 1, 0, 0, 0, 0, time.UTC)
+	clock := &sessionTestClock{now: start}
+	calls := 0
+	complete := func(ctx context.Context, request Request) (*ai.Response, error) {
+		calls++
+		if calls == 1 {
+			deadline, _ := ctx.Deadline()
+			clock.advance(deadline)
+			return nil, ctx.Err()
+		}
+		if len(request.Tools) != 0 {
+			t.Fatal("an answer correction resumed reading")
+		}
+		clock.advance(clock.now.Add(time.Second))
+		_, response := answer(`{"file":"README.md"}`, 0)(wireRequest{})
+		out := response.(ai.Response)
+		return &out, nil
+	}
+	result, err := runSession(t.Context(), complete, SessionOrder{Root: repo(t), MaxTurns: 6, Wall: 45 * time.Second,
+		AnswerReserve: 22500 * time.Millisecond, Policy: testPolicy, Schema: findingSchema}, clock.runtime())
+	if err != nil || result.Failed == "" || len(result.JSON) != 0 || calls != 2+testPolicy.FollowUps {
+		t.Fatalf("invalid answers were accepted or corrections were unbounded: %+v, %v, %d calls", result, err, calls)
+	}
+}

internal/manual/chat/review.md

@@ -45,7 +45,7 @@ Its stages are on the rail and head the lines of its page:
 6. **obligations** and **coverage**: lists what the change must do and checks it, and looks for gaps.
 7. **report**.
 
-Each reviewer is an agent session that reads the checkout, told it is part of a code review, with four read-only tools — read a file, list a folder, find files by name, look for text in files — for as many turns and minutes as its part of the review is given — from ten turns for the smallest to thirty for a reviewer — after which it is made to answer. At a shell, `--max-turns` and `--session-wall` set one limit for every reviewer instead. A review of a mid-sized pull request commonly takes half an hour to an hour.
+Each reviewer is an agent session that reads the checkout, told it is part of a code review, with four read-only tools — read a file, list a folder, find files by name, look for text in files — within the turns and minutes its part of the review is given — from ten turns for the smallest to thirty for a reviewer. It reserves thirty seconds of its time limit for the answer, or half the remaining time when the limit is shorter than a minute. At the reading cutoff it stops reading and answers from what it has already found; the answer must still finish within the original time limit. At a shell, `--max-turns` and `--session-wall` set one limit for every reviewer instead. A review of a mid-sized pull request commonly takes half an hour to an hour.
 
 ## What /review shows while it runs — each reviewer's line, what it found, which files
 
@@ -60,6 +60,8 @@ When it finishes, the chat is handed a short account whose first line says what
 
 A shell run's record folder is under `~/.codeaf/v3/carried/review/`, one folder per run.
 
+If the pipeline stops before it returns an assembled review, the run keeps `review-failure.json` and `review-failure.md` instead. They record the stage, reason, elapsed time, recorded spend and failed call counts. They contain no review and cannot be posted as one; the run still ends as failed or stopped on its ceiling.
+
 ## Post the review to GitHub — comment on the pull request, request changes, /review post
 
 `review` never posts during a review. When a review found something, the chat offers to post it and waits; only after you say yes does it propose `review` with the brief `post <the review-report.json path>`, which you can also type yourself: `/review post ~/.codeaf/v3/…/review-report.json` (the record folder works too). That run posts the review **as it stands** — its summary, its inline comments, and its event (a request for changes when something blocks, a comment when nothing does, an approval when it found nothing) — on the commit that was reviewed, even if the pull request has moved since. Posting needs a token that can write to the repository (see *Private repositories*); without one it ends `review did not finish: nothing was posted: posting to owner/repo#123 needs a GitHub token; set GH_TOKEN or sign in with gh auth login`. GitHub refuses a request for changes on your own pull request, so there it goes as a comment, and the account says so.