From 782595d5f711ed38e27843f857b862a6f24723cc Mon Sep 17 00:00:00 2001 From: ZeroPoint95 <329227198+ZeroPoint95@users.noreply.github.com> Date: Tue, 13 Oct 2026 19:01:29 -0400 Subject: [PATCH 1/3] fix(review): reserve answer time within agent deadlines --- internal/agentsession/app.go | 5 +- internal/agentsession/session.go | 51 ++++- .../agentsession/session_deadline_test.go | 201 ++++++++++++++++++ internal/manual/chat/review.md | 2 +- internal/praf/limits.go | 4 + internal/praf/praf_test.go | 10 +- internal/praf/review.go | 22 ++ internal/praf/session_deadline_test.go | 43 ++++ 8 files changed, 327 insertions(+), 11 deletions(-) create mode 100644 internal/agentsession/session_deadline_test.go create mode 100644 internal/praf/session_deadline_test.go diff --git a/internal/agentsession/app.go b/internal/agentsession/app.go index 69b8cde265..91633917fd 100644 --- a/internal/agentsession/app.go +++ b/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 { diff --git a/internal/agentsession/session.go b/internal/agentsession/session.go index 1cba673c10..a79dfb14ed 100644 --- a/internal/agentsession/session.go +++ b/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,8 +123,6 @@ 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 @@ -104,7 +130,8 @@ func RunSession(ctx context.Context, client *Client, order SessionOrder) (Sessio // 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)) } @@ -112,10 +139,22 @@ func RunSession(ctx context.Context, client *Client, order SessionOrder) (Sessio 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 } diff --git a/internal/agentsession/session_deadline_test.go b/internal/agentsession/session_deadline_test.go new file mode 100644 index 0000000000..eff569a46d --- /dev/null +++ b/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) + } +} diff --git a/internal/manual/chat/review.md b/internal/manual/chat/review.md index 5d1d3a7e37..e32cb42c71 100644 --- a/internal/manual/chat/review.md +++ b/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 diff --git a/internal/praf/limits.go b/internal/praf/limits.go index 0299311f89..1065e11245 100644 --- a/internal/praf/limits.go +++ b/internal/praf/limits.go @@ -90,6 +90,10 @@ func limitsFor(o options) func(secappx.HarnessOptions) agentsession.Limits { if o.sessionWall > 0 { limits.Wall = o.sessionWall } + // Leave time for the structured answer inside the requested wall. + // Short sessions split reading and answering evenly; longer sessions + // reserve thirty seconds. The shared loop also honors a shorter caller. + limits.AnswerReserve = min(30*time.Second, limits.Wall/2) return limits } } diff --git a/internal/praf/praf_test.go b/internal/praf/praf_test.go index 8ca141c423..12c85b830e 100644 --- a/internal/praf/praf_test.go +++ b/internal/praf/praf_test.go @@ -502,14 +502,18 @@ func TestEachAgentHasItsOwnLimits(t *testing.T) { } } own := limitsFor(options{}) - if got := own(secappx.HarnessOptions{Label: reasoners.LabelReviewer}); got != agentLimits[reasoners.LabelReviewer] { + wantReviewer := agentLimits[reasoners.LabelReviewer] + wantReviewer.AnswerReserve = 30 * time.Second + if got := own(secappx.HarnessOptions{Label: reasoners.LabelReviewer}); got != wantReviewer { t.Errorf("reviewer limits = %+v", got) } - if got := own(secappx.HarnessOptions{Label: "something else"}); got != otherLimits { + wantOther := otherLimits + wantOther.AnswerReserve = 30 * time.Second + if got := own(secappx.HarnessOptions{Label: "something else"}); got != wantOther { t.Errorf("an unknown agent's limits = %+v", got) } overridden := limitsFor(options{maxTurns: 7, sessionWall: time.Minute}) - if got := overridden(secappx.HarnessOptions{Label: reasoners.LabelLens}); got != (agentsession.Limits{Turns: 7, Wall: time.Minute}) { + if got := overridden(secappx.HarnessOptions{Label: reasoners.LabelLens}); got != (agentsession.Limits{Turns: 7, Wall: time.Minute, AnswerReserve: 30 * time.Second}) { t.Errorf("--max-turns and --session-wall do not override: %+v", got) } fs := flag.NewFlagSet("review run", flag.ContinueOnError) diff --git a/internal/praf/review.go b/internal/praf/review.go index ac77c936b8..1d128d0237 100644 --- a/internal/praf/review.go +++ b/internal/praf/review.go @@ -12,6 +12,7 @@ import ( "os/exec" "path/filepath" "strings" + "sync" "time" "github.com/Agent-Field/codeaf/internal/agentsession" @@ -209,6 +210,7 @@ func runReview(ctx context.Context, host delegate.Host, request Request, o optio Sessions: o.sessions, Calls: callsAtOnce, MaxTurns: otherLimits.Turns, SessionWall: otherLimits.Wall, Limits: limitsFor(o), Policy: sessionPolicy, + Watch: &agentsession.Watch{Session: sessionFailureNotes(notes)}, }) if err != nil { return delegate.Ending{Status: delegate.StatusFail, Message: "its agent sessions could not start: " + err.Error()} @@ -289,6 +291,26 @@ func runReview(ctx context.Context, host delegate.Host, request Request, o optio } } +// sessionFailureNotes keeps the label and elapsed time with a failed agent, +// so a later diagnostic can distinguish its own wall from the whole run's. +// Concurrent reviewers share one notes stream. +func sessionFailureNotes(notes io.Writer) func(string, agentsession.SessionResult, error) { + var mu sync.Mutex + return func(label string, result agentsession.SessionResult, err error) { + failure := result.Failed + if err != nil { + failure = err.Error() + } + if failure == "" { + return + } + mu.Lock() + defer mu.Unlock() + _, _ = fmt.Fprintf(notes, "[review] agent %s failed after %s (%d turns, %d reads): %s\n", + label, result.Duration.Round(time.Millisecond), result.Turns, result.Tools, firstSentence(failure)) + } +} + // failedReview is the ending of a review the pipeline gave up on, by why. // // ITS SENTENCE IS THE REASON ALONE. codeaf says the program's name and how diff --git a/internal/praf/session_deadline_test.go b/internal/praf/session_deadline_test.go new file mode 100644 index 0000000000..f15cef1c48 --- /dev/null +++ b/internal/praf/session_deadline_test.go @@ -0,0 +1,43 @@ +package praf + +import ( + "bytes" + "context" + "strings" + "sync" + "testing" + "time" + + "github.com/Agent-Field/codeaf/internal/agentsession" + secappx "github.com/Agent-Field/codeaf/internal/agentsession/appx" +) + +func TestReviewAnswerReserveStaysInsideTheRequestedSessionWall(t *testing.T) { + for _, wall := range []time.Duration{time.Second, 45 * time.Second, 10 * time.Minute} { + got := limitsFor(options{sessionWall: wall})(secappx.HarnessOptions{Label: "reviewer"}) + if got.Wall != wall || got.AnswerReserve != min(30*time.Second, wall/2) { + t.Fatalf("requested %s, got %+v", wall, got) + } + } +} + +func TestReviewFailedAgentsKeepLabelsTimesAndReasons(t *testing.T) { + var notes bytes.Buffer + watch := sessionFailureNotes(¬es) + watch("successful", agentsession.SessionResult{Duration: time.Second}, nil) + if notes.Len() != 0 { + t.Fatal("successful sessions emitted failure evidence") + } + var group sync.WaitGroup + for _, label := range []string{"anatomy", "coverage"} { + group.Go(func() { + watch(label, agentsession.SessionResult{Duration: 45 * time.Second, Turns: 6, Tools: 4}, context.DeadlineExceeded) + }) + } + group.Wait() + for _, want := range []string{"agent anatomy failed after 45s (6 turns, 4 reads): context deadline exceeded", "agent coverage failed after 45s (6 turns, 4 reads): context deadline exceeded"} { + if !strings.Contains(notes.String(), want) { + t.Fatalf("missing failure evidence %q in %q", want, notes.String()) + } + } +} From ceb701f8a92091cff5a2ce6b2973efaf6e00ee22 Mon Sep 17 00:00:00 2001 From: ZeroPoint95 <329227198+ZeroPoint95@users.noreply.github.com> Date: Tue, 13 Oct 2026 19:33:52 -0400 Subject: [PATCH 2/3] fix(review): retain non-postable pipeline failure records --- internal/manual/chat/review.md | 2 ++ internal/praf/failure.go | 55 ++++++++++++++++++++++++++++ internal/praf/failure_test.go | 65 ++++++++++++++++++++++++++++++++++ internal/praf/review.go | 19 +++++++++- 4 files changed, 140 insertions(+), 1 deletion(-) create mode 100644 internal/praf/failure.go create mode 100644 internal/praf/failure_test.go diff --git a/internal/manual/chat/review.md b/internal/manual/chat/review.md index e32cb42c71..bf276a0966 100644 --- a/internal/manual/chat/review.md +++ b/internal/manual/chat/review.md @@ -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 `, 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. diff --git a/internal/praf/failure.go b/internal/praf/failure.go new file mode 100644 index 0000000000..0d09f52037 --- /dev/null +++ b/internal/praf/failure.go @@ -0,0 +1,55 @@ +package praf + +import ( + "encoding/json" + "fmt" + "os" + "path/filepath" + "time" +) + +// failureRecord is evidence of a run that produced no assembled review. +// It deliberately has no Saved.Program or review payload, so post rejects it. +type failureRecord struct { + Kind string `json:"kind"` + PullRequest string `json:"pull_request_url"` + Stage string `json:"stage"` + Reason string `json:"reason"` + Failure string `json:"failure"` + Elapsed float64 `json:"elapsed_seconds"` + CostUSD float64 `json:"cost_usd"` + Sessions int `json:"agent_sessions"` + Calls int `json:"single_calls"` + FailedSession int `json:"failed_agent_sessions"` + FailedCall int `json:"failed_single_calls"` +} + +func writeFailureRecord(folder string, record failureRecord) ([]string, error) { + if folder == "" { + return nil, nil + } + if err := os.MkdirAll(folder, 0o700); err != nil { + return nil, err + } + data, err := json.MarshalIndent(record, "", " ") + if err != nil { + return nil, err + } + var written []string + for _, file := range []struct { + name string + body []byte + }{ + {"review-failure.json", append(data, '\n')}, + {"review-failure.md", []byte(fmt.Sprintf("# Review did not finish\n\nPull request: %s\n\nStage: %s\n\n%s\n\nFailure: %s\n\nElapsed: %s; recorded spend: $%.4f.\n\nRecorded agent sessions: %d (%d failed); single calls: %d (%d failed). These counts and spend were recorded when the pipeline ended.\n\nNo assembled review was returned. This record cannot be posted as a review. Nothing was posted to GitHub.\n", + record.PullRequest, record.Stage, record.Reason, record.Failure, (time.Duration(record.Elapsed * float64(time.Second))).Round(time.Millisecond), + record.CostUSD, record.Sessions, record.FailedSession, record.Calls, record.FailedCall))}, + } { + path := filepath.Join(folder, file.name) + if err := os.WriteFile(path, file.body, 0o600); err != nil { + return written, err + } + written = append(written, path) + } + return written, nil +} diff --git a/internal/praf/failure_test.go b/internal/praf/failure_test.go new file mode 100644 index 0000000000..d81df601f5 --- /dev/null +++ b/internal/praf/failure_test.go @@ -0,0 +1,65 @@ +package praf + +import ( + "context" + "encoding/json" + "io" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/Agent-Field/codeaf/internal/delegate" + "github.com/Agent-Field/codeaf/internal/praf/orch" + "github.com/Agent-Field/codeaf/internal/praf/schemas" +) + +func TestStoppedReviewKeepsANonPostableFailureRecord(t *testing.T) { + r, _ := testReviewer(&fakeSessions{failed: "the reply holds no JSON object"}, func(ctx context.Context, deps orch.Deps) (schemas.ReviewResult, error) { + _, _ = deps.App.Harness(ctx, "review", nil, nil, appxOptions()) + return schemas.ReviewResult{}, context.Canceled + }) + host := newTestHost(t) + end := runReview(t.Context(), host, ReadBrief("o/r#7", nil), defaults(), io.Discard, r) + if end.Status != delegate.StatusFail || !strings.Contains(end.Deliverable, "No assembled review") { + t.Fatalf("failure changed into a review: %+v", end) + } + path := filepath.Join(host.records, "review-failure.json") + data, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + var record failureRecord + if err := json.Unmarshal(data, &record); err != nil { + t.Fatal(err) + } + if record.Kind != "review-failure" || record.PullRequest != "https://github.com/o/r/pull/7" || record.Failure != "context canceled" || record.Sessions != 1 || record.FailedSession != 1 || record.Elapsed <= 0 { + t.Fatalf("lost failure evidence: %+v", record) + } + if info, _ := os.Stat(path); info.Mode().Perm() != 0o600 { + t.Fatal("failure record is not owner-readable only") + } + if _, _, err := readSaved(path); err == nil { + t.Fatal("a failure record could be posted as a review") + } + if _, err := os.Stat(filepath.Join(host.records, reportJSON)); !os.IsNotExist(err) { + t.Fatal("a failed pipeline wrote a completed-review payload") + } +} + +func TestUnwritableFailureRecordDoesNotChangeTheFailedEnding(t *testing.T) { + r, _ := testReviewer(&fakeSessions{}, func(context.Context, orch.Deps) (schemas.ReviewResult, error) { + return schemas.ReviewResult{}, context.Canceled + }) + host := newTestHost(t) + blocked := filepath.Join(t.TempDir(), "file") + if err := os.WriteFile(blocked, []byte("occupied"), 0o600); err != nil { + t.Fatal(err) + } + host.records = blocked + var notes strings.Builder + end := runReview(t.Context(), host, ReadBrief("o/r#7", nil), defaults(), ¬es, r) + if end.Status != delegate.StatusFail || end.Extra != nil || !strings.Contains(notes.String(), "could not save the failure record") { + t.Fatalf("saving error hid the failure: %+v, %s", end, notes.String()) + } +} diff --git a/internal/praf/review.go b/internal/praf/review.go index 1d128d0237..612f129386 100644 --- a/internal/praf/review.go +++ b/internal/praf/review.go @@ -232,7 +232,24 @@ func runReview(ctx context.Context, host delegate.Host, request Request, o optio cost, sessionCount, callCount := sessions.Spent() if err != nil { _, _ = fmt.Fprintf(notes, "[review] the review ended without a result: %v\n", err) - return failedReview(ctx, reviewCtx, t, err, cost) + ending := failedReview(ctx, reviewCtx, t, err, cost) + if recorder, ok := host.(delegate.Recorder); ok { + calls := review.stats() + files, saveErr := writeFailureRecord(recorder.Records(), failureRecord{ + Kind: "review-failure", PullRequest: t.URL(), Stage: stageWords[tracker.stage()], + Reason: ending.Message, Failure: firstSentence(err.Error()), Elapsed: time.Since(started).Seconds(), + CostUSD: cost, Sessions: calls.SessionTotal, Calls: calls.Total - calls.SessionTotal, + FailedSession: calls.SessionFailed, FailedCall: calls.Failed - calls.SessionFailed, + }) + if saveErr != nil { + _, _ = fmt.Fprintf(notes, "[review] could not save the failure record: %v\n", saveErr) + } + if len(files) > 0 { + ending.Deliverable = "No assembled review was returned. Nothing was posted to GitHub.\nFailure record: " + strings.Join(files, "\n") + ending.Extra = map[string]any{"failure_records": files} + } + } + return ending } calls := review.stats() From ea89303d331aaa4befea2f7a706cca721e2d5336 Mon Sep 17 00:00:00 2001 From: ZeroPoint95 <329227198+ZeroPoint95@users.noreply.github.com> Date: Tue, 13 Oct 2026 19:45:32 -0400 Subject: [PATCH 3/3] docs(changes): record review deadline and failure repairs for #1817 --- .../unreleased/1817-review-answer-deadline.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 docs/changes/unreleased/1817-review-answer-deadline.md diff --git a/docs/changes/unreleased/1817-review-answer-deadline.md b/docs/changes/unreleased/1817-review-answer-deadline.md new file mode 100644 index 0000000000..22050e99db --- /dev/null +++ b/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.