Add review workflows, tree-shaped workflows and a workflow editor
This commit is contained in:
@@ -72,7 +72,7 @@ func (m *Manager) MergeAfterReview(ctx context.Context, task api.Task, user api.
|
||||
}
|
||||
|
||||
func (m *Manager) clearMergeAfterReview(ctx context.Context, taskID string) {
|
||||
_, err := m.store.TakeMergeAfterReview(ctx, taskID, "")
|
||||
_, err := m.store.TakeMergeAfterReview(ctx, taskID, nil)
|
||||
if err != nil {
|
||||
slog.Error("clear merge after review", "task", taskID, "err", err)
|
||||
}
|
||||
@@ -96,7 +96,27 @@ func (m *Manager) reviewFinished(reviewID string) {
|
||||
slog.Error("load finished review", "review", reviewID, "err", err)
|
||||
return
|
||||
}
|
||||
mar, err := m.store.TakeMergeAfterReview(ctx, r.TaskId, r.Id)
|
||||
if m.advanceReviews(ctx, r) {
|
||||
return
|
||||
}
|
||||
reviews, err := m.snapshotReviews(ctx, r.TaskId, r.SnapshotN)
|
||||
if err != nil {
|
||||
slog.Error("list snapshot reviews", "task", r.TaskId, "err", err)
|
||||
return
|
||||
}
|
||||
settled, err := m.reviewsSettled(ctx, reviews)
|
||||
if err != nil {
|
||||
slog.Error("check snapshot reviews settled", "task", r.TaskId, "err", err)
|
||||
return
|
||||
}
|
||||
if !settled {
|
||||
return
|
||||
}
|
||||
ids := []string{}
|
||||
for _, sr := range reviews {
|
||||
ids = append(ids, sr.Id)
|
||||
}
|
||||
mar, err := m.store.TakeMergeAfterReview(ctx, r.TaskId, ids)
|
||||
if err != nil {
|
||||
slog.Error("take merge after review", "task", r.TaskId, "err", err)
|
||||
return
|
||||
@@ -104,7 +124,7 @@ func (m *Manager) reviewFinished(reviewID string) {
|
||||
if mar.By == "" {
|
||||
return
|
||||
}
|
||||
reason := m.startMergeAfterReview(ctx, r, mar)
|
||||
reason := m.startMergeAfterReview(ctx, r, reviews, mar)
|
||||
if reason == "" {
|
||||
slog.Info("merge after review started", "task", r.TaskId, "review", r.Id, "by", mar.By)
|
||||
return
|
||||
@@ -121,16 +141,19 @@ func (m *Manager) reviewFinished(reviewID string) {
|
||||
}
|
||||
}
|
||||
|
||||
func (m *Manager) startMergeAfterReview(ctx context.Context, r api.Review, mar store.MergeAfterReview) string {
|
||||
switch {
|
||||
case r.State == api.ReviewStateFailed:
|
||||
return "the review failed"
|
||||
case r.State != api.ReviewStateDone:
|
||||
return fmt.Sprintf("the review ended %s", r.State)
|
||||
}
|
||||
func (m *Manager) startMergeAfterReview(ctx context.Context, r api.Review, reviews []api.Review, mar store.MergeAfterReview) string {
|
||||
findings := []any{}
|
||||
if r.Result != nil {
|
||||
findings, _ = (*r.Result)["findings"].([]any)
|
||||
for _, sr := range reviews {
|
||||
switch {
|
||||
case sr.State == api.ReviewStateFailed:
|
||||
return fmt.Sprintf("the %q review failed", sr.Stage)
|
||||
case sr.State != api.ReviewStateDone:
|
||||
return fmt.Sprintf("the %q review ended %s", sr.Stage, sr.State)
|
||||
}
|
||||
if sr.Result != nil {
|
||||
f, _ := (*sr.Result)["findings"].([]any)
|
||||
findings = append(findings, f...)
|
||||
}
|
||||
}
|
||||
switch len(findings) {
|
||||
case 0:
|
||||
|
||||
@@ -1,36 +1,41 @@
|
||||
The YAML format of a workflow's file, which read_workflow returns and save_workflow takes: stages, after, base, model and teams.
|
||||
|
||||
# Workflow files
|
||||
|
||||
A workflow's file is YAML: an optional `description`, then `stages`. The workflow's name is not in the file. Unknown fields are refused.
|
||||
A workflow's file is YAML: an optional `description`, an optional `kind`, then `steps`, which run in order. The workflow's name is not in the file. Unknown fields are refused.
|
||||
|
||||
| Field | Meaning |
|
||||
`kind` is `task` (the default) or `review`. A task workflow runs on a brief, each stage as its own task. A review workflow is what a project names (with `set_review_workflow`) to review its tasks' snapshots: each stage is one reviewer agent on its `model`, given the task's change and its stage's `prompt` (what to look for); a stage after others also gets their reviews. Its stages can't use `agents`, and it may end with several stages.
|
||||
|
||||
Each step is exactly one of:
|
||||
|
||||
| Step | Meaning |
|
||||
|---|---|
|
||||
| `stage` | A stage, by its name: one line, unique in the workflow. It runs as its own task, with `model` or `agents`, and `prompt`. |
|
||||
| `order` | A list of steps run one after another. |
|
||||
| `parallel` | A list of branches run at the same time, each a step (an `order` for a branch of several). |
|
||||
|
||||
A stage gets the outputs of the stages that end the step before it: the last stage of an `order`, or the last stage of every branch of a `parallel`. A stage with nothing before it starts from the brief.
|
||||
|
||||
| Stage field | Meaning |
|
||||
|---|---|
|
||||
| `name` | One line, unique in the workflow. |
|
||||
| `after` | The names of the stages whose outputs it gets; it starts once all of them have finished. Absent means the previous stage in the list, and for the first stage none, so a file without `after` is a chain. `[]` means it starts from the brief. |
|
||||
| `base` | `parent`, the default: branch from the commit its one `after` stage finished at, or for a stage with none, from where the workflow was started. `main`: branch from the default branch. Refused on a stage with more than one `after` stage, which always branches from the default branch and is told the branches it can merge. |
|
||||
| `model` | Also picks the vendor: `gpt-*` models run on Codex, `gemini-*` models on Antigravity, every other model on Claude Code. |
|
||||
| `prompt` | The stage agent's instructions. |
|
||||
| `agents` | In place of `model`: a list of at least two, each with `name`, `model` and `prompt`, that runs the stage as a team. They work in the stage's one task and worktree and talk through a shared board; each gets the stage's `prompt` and then its own, and the first agent finishes the stage. |
|
||||
|
||||
Every stage runs as its own task. Exactly one stage is final (no other stage comes after it), and cycles are refused.
|
||||
Where a stage's task branches from follows from the shape. A stage after exactly one stage starts from the commit that stage finished at. A stage after a `parallel` starts from the default branch and is told the branches it can merge. A stage with nothing before it starts where the workflow was started. The workflow must end with one stage, which finishes it. An `order` or `parallel` can't be empty.
|
||||
|
||||
```yaml
|
||||
description: Build a change, review and test it in parallel, then fix what they found.
|
||||
stages:
|
||||
- name: Build
|
||||
steps:
|
||||
- stage: Build
|
||||
model: claude-opus-5-5
|
||||
prompt: Implement the brief.
|
||||
- name: Review
|
||||
after: [Build]
|
||||
model: gpt-6.1-sol
|
||||
prompt: Review the Build stage's work and list the problems.
|
||||
- name: Test
|
||||
after: [Build]
|
||||
model: gemini-4-argon-high
|
||||
prompt: Exercise the change and report what fails.
|
||||
- name: Fix
|
||||
after: [Review, Test]
|
||||
- parallel:
|
||||
- stage: Review
|
||||
model: gpt-6.1-sol
|
||||
prompt: Review the Build stage's work and list the problems.
|
||||
- stage: Test
|
||||
model: gemini-4-argon-high
|
||||
prompt: Exercise the change and report what fails.
|
||||
- stage: Fix
|
||||
agents:
|
||||
- name: fixer
|
||||
model: claude-opus-5-5
|
||||
|
||||
@@ -29,11 +29,11 @@ const leadGuidance = `You are the human's lead agent in byre, shown in the left
|
||||
- Questions from task agents: answer one yourself with answer_question only when the answer is clear, for example the agent's recommendation is plainly right given what the human has said, the human already decided it, or one of your notes settles it outright. Otherwise show it to the human with your recommendation and wait. When the human tells you an answer, give it with answer_question. Say which questions you answered and how, naming the note by its id when one decided it.
|
||||
- Notes: you learn how the human works and keep it with remember, forget and list_notes. byre adds every note to your instructions at the start of each lead session, so they outlast compaction; the human can read and delete them in the Settings tab. Write a note when the human shows a priority, a preference or the reasoning behind a choice that will recur: an answer that departs from the agent's recommendation (the answered signal shows both), a correction or overrule of you or a task agent, or a statement of how they want things done. Record the choice and the why, in their terms, general enough to apply to the next case and specific enough to act on, for example "Prefers replacing a state label over adding a tag beside it: keeps the picker uncluttered." File each note under the project it is about with project_id when it concerns that project's code, UI, docs or process, and with no project_id only when it holds for how the human works everywhere; apply a project's notes only to that project's tasks. Task agents start with their creator's every-project notes and their project's, and suggest new ones with suggest_note, which reaches you as a signal: save a suggestion only when it holds and your notes don't already cover it, choosing its project yourself, and otherwise drop it; neither needs a word to the human. Don't note one-off facts about a single task, and don't duplicate: update an existing note with remember and its id instead. When the human contradicts a note, update it or forget it. Before recommending anything, check your notes, and when one shaped the recommendation, cite its id.
|
||||
- Finished turns: read them and show the human only what matters: work finished and ready to merge, problems, decisions made. Stay quiet about routine progress.
|
||||
- Reviews: after each new snapshot of a task, a reviewer agent reviews the change, on the human's review model, or their second review model when the review model wrote the work. The results are in the task's transcript, where its agent answers each finding with respond_to_finding (fixing, or ignoring with a reason), and read_transcript shows each review's findings. They are for the human to look at when they want; mention one only when its findings matter to what the human is deciding.
|
||||
- Reviews: after each new snapshot of a task, the stages of its project's review workflow (a workflow of kind review, set per project with set_review_workflow) review the change, each a reviewer agent on its stage's model; a stage after others gets their reviews. A project with no review workflow isn't reviewed. The results are in the task's transcript, where its agent answers each finding with respond_to_finding (fixing, or ignoring with a reason), and read_transcript shows each review's findings. They are for the human to look at when they want; mention one only when its findings matter to what the human is deciding.
|
||||
- Dig into context with list_tasks, read_transcript, read_inbox and read_status (vendor rate limits and agent health), and with your own Read, Grep and Bash on a task's worktree path (from list_tasks). Don't edit files in task worktrees; ask the task's agent with send_message.
|
||||
- You can create tasks, start TODO items, message, interrupt and compact task agents, and merge or discard tasks. Merge pushes to the default branch and Discard destroys work: do either only when the human asks for it.
|
||||
- Projects: when the human asks you to add a project, create it with create_project from its name and the path of its git repository on this machine.
|
||||
- Workflows: you manage the server's workflows with list_workflows, read_workflow, save_workflow and delete_workflow, and run one by giving its name to create_task or start_todo. Every stage of a workflow runs as its own task on the stage's model, once the stages it comes after have finished, so stages fan out (several stages after one) and fan in (one stage after several); a stage's agent calls finish_stage to hand its output to the stages after it, signals tell you when a stage starts and when the workflow finishes, and when the final stage finishes byre discards the run's other stage tasks, leaving the final one to merge. Whenever you show or propose a workflow, draw it as a Mermaid flowchart of its stage names only, with no models, for example a fenced mermaid block holding "flowchart LR", "Build --> Review", "Build --> Test", "Review --> Fix" and "Test --> Fix".
|
||||
- Workflows: you manage the server's workflows with list_workflows, read_workflow, save_workflow and delete_workflow, and run one by giving its name to create_task or start_todo. A workflow is a tree of steps run in order, each a stage, an order or a parallel; every stage runs as its own task on the stage's model once the step before it has finished, so stages fan out (a parallel after a stage) and fan in (a stage after a parallel); a stage's agent calls finish_stage to hand its output to the stages after it, signals tell you when a stage starts and when the workflow finishes, and when the final stage finishes byre discards the run's other stage tasks, leaving the final one to merge. Whenever you show or propose a workflow, draw it as a Mermaid flowchart of its stage names only, with no models, for example a fenced mermaid block holding "flowchart LR", "Build --> Review", "Build --> Test", "Review --> Fix" and "Test --> Fix".
|
||||
- Whenever you name a task in a message, link it as a Markdown link to #/task/<task_id>, for example [Fix login redirect](#/task/da7e487305e4). byre draws it in that task's colour, and clicking it shows the task in the right pane. To open a specific tab, append it: #/task/<task_id>/<tab>. When you point the human at a question to answer or a finished turn, link to #/task/<task_id>/transcript.
|
||||
- Keep messages short; the human reads them in half the screen. Put long detail in collapsed sections: a line <details><summary>Title</summary>, the Markdown content, then a line </details>.
|
||||
- You are long-running. Your context window is large. byre sends you a signal once your context passes a quarter of its window; then call compact_self at a logical point (a piece of work wrapped up, a topic closed), and not before; when the turn ends, byre replaces you with a fresh session that starts from your handoff.
|
||||
@@ -564,7 +564,7 @@ func (m *Manager) signalText(ctx context.Context, ev api.Event) (string, error)
|
||||
return fmt.Sprintf("%s's merge was requested by %s and is held until the merge in progress in its project is done.", m.taskLabel(ctx, deref(ev.TaskId)), ev.Actor), nil
|
||||
|
||||
case "task.merge_after_review":
|
||||
return fmt.Sprintf("%s was set by %s to merge once the review of snapshot %v finishes with no findings.", m.taskLabel(ctx, deref(ev.TaskId)), ev.Actor, ev.Payload["snapshot_n"]), nil
|
||||
return fmt.Sprintf("%s was set by %s to merge once the reviews of snapshot %v finish with no findings.", m.taskLabel(ctx, deref(ev.TaskId)), ev.Actor, ev.Payload["snapshot_n"]), nil
|
||||
|
||||
case "task.merge_after_review_cancelled":
|
||||
return fmt.Sprintf("%s's merge after review was cancelled by %s.", m.taskLabel(ctx, deref(ev.TaskId)), ev.Actor), nil
|
||||
@@ -613,7 +613,7 @@ func (m *Manager) signalText(ctx context.Context, ev api.Event) (string, error)
|
||||
return "", err
|
||||
}
|
||||
if ev.Kind == "review.started" {
|
||||
return fmt.Sprintf("%s started a review of snapshot %d.", m.taskLabel(ctx, r.TaskId), r.SnapshotN), nil
|
||||
return fmt.Sprintf("%s started the %s.", m.taskLabel(ctx, r.TaskId), reviewName(r)), nil
|
||||
}
|
||||
return fmt.Sprintf("%s %s", m.taskLabel(ctx, r.TaskId), reviewLine(r)), nil
|
||||
|
||||
|
||||
@@ -53,7 +53,7 @@ func (m *Manager) mergeNote(ctx context.Context, task api.Task, login string) (s
|
||||
if len(reviews) > 0 {
|
||||
b.WriteString("\n## Reviews\n")
|
||||
for _, r := range reviews {
|
||||
fmt.Fprintf(b, "\n### Snapshot %d, %s %s: %s\n", r.SnapshotN, r.Vendor, r.Model, r.State)
|
||||
fmt.Fprintf(b, "\n### Snapshot %d, reviewer %q, %s %s: %s\n", r.SnapshotN, r.Stage, r.Vendor, r.Model, r.State)
|
||||
if r.Error != "" {
|
||||
fmt.Fprintf(b, "\n%s\n", r.Error)
|
||||
}
|
||||
|
||||
+17
-11
@@ -174,24 +174,30 @@ func (m *Manager) tryPromptReady(ctx context.Context, taskID string, fromReview
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
reviews, err := m.store.ListReviews(ctx, taskID)
|
||||
reviews, err := m.snapshotReviews(ctx, taskID, latest.N)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
var review *api.Review
|
||||
for _, r := range reviews {
|
||||
if r.SnapshotN == latest.N {
|
||||
review = &r
|
||||
if len(reviews) == 0 {
|
||||
project, err := m.store.GetProject(ctx, task.ProjectId)
|
||||
if err != nil || project.ReviewWorkflow != "" {
|
||||
return err
|
||||
}
|
||||
}
|
||||
if review == nil || (review.State != api.ReviewStateDone && review.State != api.ReviewStateSkipped) {
|
||||
return nil
|
||||
settled, err := m.reviewsSettled(ctx, reviews)
|
||||
if err != nil || !settled {
|
||||
return err
|
||||
}
|
||||
if fromReview && review.State == api.ReviewStateDone && review.Result != nil {
|
||||
findings, _ := (*review.Result)["findings"].([]any)
|
||||
if len(findings) > 0 {
|
||||
for _, r := range reviews {
|
||||
if r.State != api.ReviewStateDone && r.State != api.ReviewStateSkipped {
|
||||
return nil
|
||||
}
|
||||
if fromReview && r.Result != nil {
|
||||
findings, _ := (*r.Result)["findings"].([]any)
|
||||
if len(findings) > 0 {
|
||||
return nil
|
||||
}
|
||||
}
|
||||
}
|
||||
due, err := m.store.ReadyPromptDue(ctx, taskID)
|
||||
if err != nil || !due {
|
||||
@@ -202,7 +208,7 @@ func (m *Manager) tryPromptReady(ctx context.Context, taskID string, fromReview
|
||||
return err
|
||||
}
|
||||
prompt := readyPrompt
|
||||
if review.State == api.ReviewStateSkipped {
|
||||
if len(reviews) == 1 && reviews[0].State == api.ReviewStateSkipped {
|
||||
prompt = readyPromptUnchanged
|
||||
}
|
||||
_, err = m.Send(ctx, sess.Id, "byre", prompt, "", "")
|
||||
|
||||
+231
-55
@@ -104,12 +104,61 @@ func (m *Manager) afterSnapshot(snap *api.Snapshot) {
|
||||
return
|
||||
}
|
||||
}
|
||||
err = m.startReviewer(ctx, task, *snap)
|
||||
err = m.startReviews(ctx, task, *snap)
|
||||
if err != nil {
|
||||
slog.Error("start review", "task", task.Id, "snapshot", snap.N, "err", err)
|
||||
}
|
||||
}
|
||||
|
||||
func (m *Manager) reviewsSettled(ctx context.Context, reviews []api.Review) (bool, error) {
|
||||
byStage := map[string]api.Review{}
|
||||
for _, r := range reviews {
|
||||
if r.State == api.ReviewStateRunning {
|
||||
return false, nil
|
||||
}
|
||||
byStage[r.Stage] = r
|
||||
}
|
||||
for _, r := range reviews {
|
||||
if r.State != api.ReviewStateDone {
|
||||
continue
|
||||
}
|
||||
def, err := m.store.ReviewDefinition(ctx, r.Id)
|
||||
if err != nil {
|
||||
return false, err
|
||||
}
|
||||
if len(def.Steps) == 0 {
|
||||
continue
|
||||
}
|
||||
for _, next := range workflow.Next(def, r.Stage) {
|
||||
if _, found := byStage[next]; found {
|
||||
continue
|
||||
}
|
||||
ready := true
|
||||
for _, a := range workflow.After(def, next) {
|
||||
ready = ready && byStage[a].State == api.ReviewStateDone
|
||||
}
|
||||
if ready {
|
||||
return false, nil
|
||||
}
|
||||
}
|
||||
}
|
||||
return true, nil
|
||||
}
|
||||
|
||||
func (m *Manager) snapshotReviews(ctx context.Context, taskID string, n int) ([]api.Review, error) {
|
||||
reviews, err := m.store.ListReviews(ctx, taskID)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
out := []api.Review{}
|
||||
for _, r := range reviews {
|
||||
if r.SnapshotN == n {
|
||||
out = append(out, r)
|
||||
}
|
||||
}
|
||||
return out, nil
|
||||
}
|
||||
|
||||
func (m *Manager) reviewDir(projectID, reviewID string) string {
|
||||
return filepath.Join(m.worktreeRoot, projectID, "review-"+reviewID)
|
||||
}
|
||||
@@ -126,54 +175,149 @@ func playwrightMCP(outputDir string) mcp.Stdio {
|
||||
}
|
||||
}
|
||||
|
||||
func (m *Manager) reviewerModel(ctx context.Context, task api.Task) (string, error) {
|
||||
creator, err := m.store.GetUser(ctx, task.CreatedBy)
|
||||
if err != nil {
|
||||
return "", err
|
||||
}
|
||||
sessions, err := m.store.ListSessions(ctx, task.Id)
|
||||
if err != nil {
|
||||
return "", err
|
||||
}
|
||||
if len(sessions) > 0 && sessions[len(sessions)-1].Model == creator.ReviewModel {
|
||||
return creator.SecondReviewModel, nil
|
||||
}
|
||||
return creator.ReviewModel, nil
|
||||
}
|
||||
|
||||
func (m *Manager) startReviewer(ctx context.Context, task api.Task, snap api.Snapshot) error {
|
||||
func (m *Manager) startReviews(ctx context.Context, task api.Task, snap api.Snapshot) error {
|
||||
project, err := m.store.GetProject(ctx, task.ProjectId)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
model, err := m.reviewerModel(ctx, task)
|
||||
defer m.failRunningReviews(ctx, task.Id, project, snap.N, fmt.Sprintf("superseded by snapshot %d", snap.N))
|
||||
if project.ReviewWorkflow == "" {
|
||||
slog.Info("review not started, the project has no review workflow", "task", task.Id, "snapshot", snap.N)
|
||||
return nil
|
||||
}
|
||||
changes, err := git.Compare(project.RepoPath, project.DefaultBranch, snap.CommitSha, snap.TreeSha, "")
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
vendor := workflow.VendorOf(model)
|
||||
prompt, err := m.reviewPrompt(ctx, task, project, snap)
|
||||
if err != nil {
|
||||
return err
|
||||
now := time.Now().UTC()
|
||||
if len(changes.Files) == 0 {
|
||||
slog.Info("review skipped, snapshot has no diff", "task", task.Id, "snapshot", snap.N)
|
||||
return m.store.CreateReview(ctx, api.Review{Id: store.NewID(), TaskId: task.Id, SnapshotN: snap.N, State: api.ReviewStateSkipped, CreatedAt: now, FinishedAt: &now}, nil)
|
||||
}
|
||||
if prompt == "" {
|
||||
now := time.Now().UTC()
|
||||
err = m.store.CreateReview(ctx, api.Review{
|
||||
|
||||
def, err := m.store.GetWorkflow(ctx, project.ReviewWorkflow)
|
||||
if err == nil && def.Kind != api.WorkflowKindReview {
|
||||
err = fmt.Errorf("it is a task workflow, not a review workflow")
|
||||
}
|
||||
if err != nil {
|
||||
return m.store.CreateReview(ctx, api.Review{
|
||||
Id: store.NewID(),
|
||||
TaskId: task.Id,
|
||||
SnapshotN: snap.N,
|
||||
State: api.ReviewStateSkipped,
|
||||
Stage: project.ReviewWorkflow,
|
||||
State: api.ReviewStateFailed,
|
||||
Error: fmt.Sprintf("project %s reviews with workflow %q, which can't run: %v", project.Name, project.ReviewWorkflow, err),
|
||||
CreatedAt: now,
|
||||
FinishedAt: &now,
|
||||
})
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
slog.Info("review skipped, snapshot has no diff", "task", task.Id, "snapshot", snap.N)
|
||||
m.failRunningReviews(ctx, task.Id, project, snap.N, fmt.Sprintf("superseded by snapshot %d", snap.N))
|
||||
return nil
|
||||
}, nil)
|
||||
}
|
||||
|
||||
for _, stage := range workflow.Starts(def) {
|
||||
err = m.startReviewer(ctx, task, project, snap, def, stage, nil)
|
||||
if err != nil {
|
||||
slog.Error("start review stage", "task", task.Id, "snapshot", snap.N, "stage", stage, "err", err)
|
||||
}
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
func (m *Manager) advanceReviews(ctx context.Context, r api.Review) bool {
|
||||
if r.State != api.ReviewStateDone {
|
||||
return false
|
||||
}
|
||||
m.reviewMu.Lock()
|
||||
defer m.reviewMu.Unlock()
|
||||
|
||||
def, err := m.store.ReviewDefinition(ctx, r.Id)
|
||||
if err != nil {
|
||||
slog.Error("load review definition", "review", r.Id, "err", err)
|
||||
return false
|
||||
}
|
||||
if len(def.Steps) == 0 {
|
||||
return false
|
||||
}
|
||||
task, project, err := m.activeTask(ctx, r.TaskId)
|
||||
if err != nil {
|
||||
return false
|
||||
}
|
||||
all, err := m.store.ListReviews(ctx, task.Id)
|
||||
if err != nil {
|
||||
slog.Error("list reviews to advance", "task", task.Id, "err", err)
|
||||
return false
|
||||
}
|
||||
byStage := map[string]api.Review{}
|
||||
for _, sr := range all {
|
||||
if sr.SnapshotN > r.SnapshotN {
|
||||
return false
|
||||
}
|
||||
if sr.SnapshotN == r.SnapshotN {
|
||||
byStage[sr.Stage] = sr
|
||||
}
|
||||
}
|
||||
snap, err := m.store.GetSnapshot(ctx, task.Id, r.SnapshotN)
|
||||
if err != nil {
|
||||
slog.Error("load snapshot to advance review", "task", task.Id, "snapshot", r.SnapshotN, "err", err)
|
||||
return false
|
||||
}
|
||||
|
||||
started := false
|
||||
for _, next := range workflow.Next(def, r.Stage) {
|
||||
if _, found := byStage[next]; found {
|
||||
continue
|
||||
}
|
||||
inputs := []api.Review{}
|
||||
for _, a := range workflow.After(def, next) {
|
||||
if in, found := byStage[a]; found && in.State == api.ReviewStateDone {
|
||||
inputs = append(inputs, in)
|
||||
}
|
||||
}
|
||||
if len(inputs) != len(workflow.After(def, next)) {
|
||||
continue
|
||||
}
|
||||
err = m.startReviewer(ctx, task, project, snap, def, next, inputs)
|
||||
if err != nil {
|
||||
slog.Error("start review stage", "task", task.Id, "snapshot", r.SnapshotN, "stage", next, "err", err)
|
||||
continue
|
||||
}
|
||||
started = true
|
||||
}
|
||||
return started
|
||||
}
|
||||
|
||||
func (m *Manager) startReviewer(ctx context.Context, task api.Task, project api.Project, snap api.Snapshot, def api.WorkflowDefinition, stage string, inputs []api.Review) (err error) {
|
||||
model := workflow.Get(def, stage).Model
|
||||
vendor := workflow.VendorOf(model)
|
||||
reviewID := store.NewID()
|
||||
recorded := false
|
||||
defer func() {
|
||||
if err == nil || recorded {
|
||||
return
|
||||
}
|
||||
now := time.Now().UTC()
|
||||
rerr := m.store.CreateReview(ctx, api.Review{
|
||||
Id: reviewID,
|
||||
TaskId: task.Id,
|
||||
SnapshotN: snap.N,
|
||||
Stage: stage,
|
||||
Vendor: string(vendor),
|
||||
Model: model,
|
||||
State: api.ReviewStateFailed,
|
||||
Error: "the reviewer didn't start: " + err.Error(),
|
||||
CreatedAt: now,
|
||||
FinishedAt: &now,
|
||||
}, &def)
|
||||
if rerr != nil {
|
||||
slog.Error("record review that didn't start", "task", task.Id, "stage", stage, "err", rerr)
|
||||
return
|
||||
}
|
||||
go m.reviewFinished(reviewID)
|
||||
}()
|
||||
|
||||
prompt, err := m.reviewPrompt(ctx, task, project, snap, def, stage, inputs)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
dir := m.reviewDir(project.Id, reviewID)
|
||||
err = git.AddDetachedWorktree(project.RepoPath, dir, snap.CommitSha)
|
||||
if err != nil {
|
||||
@@ -206,16 +350,18 @@ func (m *Manager) startReviewer(ctx context.Context, task api.Task, snap api.Sna
|
||||
Id: reviewID,
|
||||
TaskId: task.Id,
|
||||
SnapshotN: snap.N,
|
||||
Stage: stage,
|
||||
AgentSessionId: sess.Id,
|
||||
Vendor: string(vendor),
|
||||
Model: model,
|
||||
State: api.ReviewStateRunning,
|
||||
CreatedAt: time.Now().UTC(),
|
||||
})
|
||||
}, &def)
|
||||
if err != nil {
|
||||
m.endReviewer(ctx, sess.Id, project, reviewID)
|
||||
return err
|
||||
}
|
||||
recorded = true
|
||||
|
||||
_, err = m.turn(ctx, sess, task, "byre", prompt, "", nil)
|
||||
if err != nil {
|
||||
@@ -223,26 +369,22 @@ func (m *Manager) startReviewer(ctx context.Context, task api.Task, snap api.Sna
|
||||
m.failReview(ctx, reviewID, "the reviewer didn't start: "+err.Error())
|
||||
return err
|
||||
}
|
||||
slog.Info("review started", "review", reviewID, "task", task.Id, "snapshot", snap.N, "vendor", vendor, "model", model)
|
||||
m.failRunningReviews(ctx, task.Id, project, snap.N, fmt.Sprintf("superseded by snapshot %d", snap.N))
|
||||
slog.Info("review started", "review", reviewID, "task", task.Id, "snapshot", snap.N, "stage", stage, "vendor", vendor, "model", model)
|
||||
return nil
|
||||
}
|
||||
|
||||
func (m *Manager) reviewPrompt(ctx context.Context, task api.Task, project api.Project, snap api.Snapshot) (string, error) {
|
||||
func (m *Manager) reviewPrompt(ctx context.Context, task api.Task, project api.Project, snap api.Snapshot, def api.WorkflowDefinition, stage string, inputs []api.Review) (string, error) {
|
||||
changes, err := git.Compare(project.RepoPath, project.DefaultBranch, snap.CommitSha, snap.TreeSha, "")
|
||||
if err != nil {
|
||||
return "", err
|
||||
}
|
||||
if len(changes.Files) == 0 {
|
||||
return "", nil
|
||||
}
|
||||
brief, err := m.store.FirstTaskMessage(ctx, task.Id)
|
||||
if err != nil {
|
||||
return "", err
|
||||
}
|
||||
|
||||
b := &strings.Builder{}
|
||||
fmt.Fprintf(b, "Review the work of the agent on the byre task %q: look for bugs, security problems, missing or wrong tests, and anything that doesn't do what the task asked.\n\n", task.Title)
|
||||
fmt.Fprintf(b, "Review the work of the agent on the byre task %q, as reviewer %q of the project's review workflow %q.\n\n%s\n\n", task.Title, stage, project.ReviewWorkflow, strings.TrimSpace(workflow.Get(def, stage).Prompt))
|
||||
fmt.Fprintf(b, "It is snapshot %d of the task, commit %s, checked out in your working directory. The change is all of the task's work, `git diff %s %s` (from the merge-base with %s); %d files changed:\n",
|
||||
snap.N, snap.CommitSha, changes.Base, snap.CommitSha, project.DefaultBranch, len(changes.Files))
|
||||
for i, f := range changes.Files {
|
||||
@@ -255,6 +397,9 @@ func (m *Manager) reviewPrompt(ctx context.Context, task api.Task, project api.P
|
||||
if brief != "" {
|
||||
fmt.Fprintf(b, "\nThe task's brief, the first message its agent got:\n\n%s\n", quote(clip(brief, 4000)))
|
||||
}
|
||||
for _, in := range inputs {
|
||||
fmt.Fprintf(b, "\n## From reviewer %q\n\n%s\n", in.Stage, reviewText(in))
|
||||
}
|
||||
b.WriteString("\nWhen you're done, call submit_review once with your review, then end your turn.")
|
||||
return b.String(), nil
|
||||
}
|
||||
@@ -370,9 +515,9 @@ func findingsMessage(r api.Review) string {
|
||||
res = *r.Result
|
||||
}
|
||||
b := &strings.Builder{}
|
||||
fmt.Fprintf(b, findingsPreamble+"%d of your work and raised the findings below. Reviewers can be wrong or miss context: check each one against the code and your brief. "+
|
||||
"Before you change anything, answer each finding with respond_to_finding (snapshot %d and the finding's number): fixing if it holds up and you'll act on it, ignoring with your reason if you're leaving it. "+
|
||||
"The human sees your answers on the findings and can overrule an ignore.", r.SnapshotN, r.SnapshotN)
|
||||
fmt.Fprintf(b, findingsPreamble+"%d of your work as reviewer %q and raised the findings below. Reviewers can be wrong or miss context: check each one against the code and your brief. "+
|
||||
"Before you change anything, answer each finding with respond_to_finding (snapshot %d, reviewer %q and the finding's number): fixing if it holds up and you'll act on it, ignoring with your reason if you're leaving it. "+
|
||||
"The human sees your answers on the findings and can overrule an ignore.", r.SnapshotN, r.Stage, r.SnapshotN, r.Stage)
|
||||
findings, _ := res["findings"].([]any)
|
||||
for i, f := range findings {
|
||||
fm, _ := f.(map[string]any)
|
||||
@@ -391,6 +536,7 @@ func findingsMessage(r api.Review) string {
|
||||
|
||||
type findingResponseArgs struct {
|
||||
Snapshot int `json:"snapshot"`
|
||||
Reviewer string `json:"reviewer"`
|
||||
Finding int `json:"finding"`
|
||||
Response api.FindingResponseResponse `json:"response"`
|
||||
Reason string `json:"reason"`
|
||||
@@ -405,11 +551,12 @@ func (m *Manager) respondToFindingTool(l *live) mcp.Tool {
|
||||
"type": "object",
|
||||
"properties": {
|
||||
"snapshot": {"type": "integer", "description": "The reviewed snapshot's number."},
|
||||
"finding": {"type": "integer", "description": "The finding's number in the review, from 1."},
|
||||
"reviewer": {"type": "string", "description": "The reviewer's name, as the findings message gives it."},
|
||||
"finding": {"type": "integer", "description": "The finding's number in that reviewer's review, from 1."},
|
||||
"response": {"type": "string", "enum": ["fixing", "ignoring"]},
|
||||
"reason": {"type": "string", "description": "One line: what you'll do about it, or why you're leaving it."}
|
||||
},
|
||||
"required": ["snapshot", "finding", "response", "reason"]
|
||||
"required": ["snapshot", "reviewer", "finding", "response", "reason"]
|
||||
}`),
|
||||
Handler: func(ctx context.Context, raw json.RawMessage) (string, error) {
|
||||
args := findingResponseArgs{}
|
||||
@@ -433,21 +580,21 @@ func (m *Manager) respondToFindingTool(l *live) mcp.Tool {
|
||||
}
|
||||
reviewID := ""
|
||||
for _, r := range reviews {
|
||||
if r.SnapshotN == args.Snapshot && r.State == api.ReviewStateDone {
|
||||
if r.SnapshotN == args.Snapshot && r.Stage == args.Reviewer && r.State == api.ReviewStateDone {
|
||||
reviewID = r.Id
|
||||
}
|
||||
}
|
||||
if reviewID == "" {
|
||||
return "", fmt.Errorf("%w: snapshot %d has no finished review", ErrInvalid, args.Snapshot)
|
||||
return "", fmt.Errorf("%w: snapshot %d has no finished review by reviewer %q", ErrInvalid, args.Snapshot, args.Reviewer)
|
||||
}
|
||||
err = m.store.SetFindingResponse(ctx, reviewID, args.Finding-1, api.FindingResponse{Response: args.Response, Reason: reason}, agentActor)
|
||||
if errors.Is(err, store.ErrNotFound) {
|
||||
return "", fmt.Errorf("%w: the review of snapshot %d has no finding %d", ErrInvalid, args.Snapshot, args.Finding)
|
||||
return "", fmt.Errorf("%w: reviewer %q's review of snapshot %d has no finding %d", ErrInvalid, args.Reviewer, args.Snapshot, args.Finding)
|
||||
}
|
||||
if err != nil {
|
||||
return "", err
|
||||
}
|
||||
return fmt.Sprintf("Recorded: %s finding %d of snapshot %d's review.", args.Response, args.Finding, args.Snapshot), nil
|
||||
return fmt.Sprintf("Recorded: %s finding %d of reviewer %q's review of snapshot %d.", args.Response, args.Finding, args.Reviewer, args.Snapshot), nil
|
||||
},
|
||||
}
|
||||
}
|
||||
@@ -672,25 +819,55 @@ func (m *Manager) recoverReviews(ctx context.Context) error {
|
||||
m.endReviewer(ctx, r.AgentSessionId, project, r.Id)
|
||||
m.failReview(ctx, r.Id, "byre restarted before the reviewer finished")
|
||||
}
|
||||
done, err := m.store.LatestDoneStages(ctx)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
for _, id := range done {
|
||||
go m.reviewFinished(id)
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
func reviewName(r api.Review) string {
|
||||
if r.Stage == "" {
|
||||
return fmt.Sprintf("review of snapshot %d", r.SnapshotN)
|
||||
}
|
||||
return fmt.Sprintf("%q review of snapshot %d", r.Stage, r.SnapshotN)
|
||||
}
|
||||
|
||||
func reviewLine(r api.Review) string {
|
||||
if r.State == api.ReviewStateFailed {
|
||||
return fmt.Sprintf("review of snapshot %d failed: %s", r.SnapshotN, clip(r.Error, 300))
|
||||
return fmt.Sprintf("%s failed: %s", reviewName(r), clip(r.Error, 300))
|
||||
}
|
||||
if r.State == api.ReviewStateSkipped {
|
||||
return fmt.Sprintf("review of snapshot %d skipped: no changes", r.SnapshotN)
|
||||
return fmt.Sprintf("%s skipped: no changes", reviewName(r))
|
||||
}
|
||||
res := map[string]any{}
|
||||
if r.Result != nil {
|
||||
res = *r.Result
|
||||
}
|
||||
b := &strings.Builder{}
|
||||
fmt.Fprintf(b, "review of snapshot %d: %s", r.SnapshotN, clip(strings.TrimSpace(fmt.Sprint(res["summary"])), 600))
|
||||
fmt.Fprintf(b, "%s: %s", reviewName(r), clip(strings.TrimSpace(fmt.Sprint(res["summary"])), 600))
|
||||
if c, ok := res["change"].(string); ok && c != "" {
|
||||
fmt.Fprintf(b, "\n change: %s", clip(strings.TrimSpace(c), 600))
|
||||
}
|
||||
writeFindings(b, res, "\n ", 300)
|
||||
return b.String()
|
||||
}
|
||||
|
||||
func reviewText(r api.Review) string {
|
||||
res := map[string]any{}
|
||||
if r.Result != nil {
|
||||
res = *r.Result
|
||||
}
|
||||
b := &strings.Builder{}
|
||||
b.WriteString(strings.TrimSpace(fmt.Sprint(res["summary"])))
|
||||
writeFindings(b, res, "\n", 4000)
|
||||
return b.String()
|
||||
}
|
||||
|
||||
func writeFindings(b *strings.Builder, res map[string]any, lead string, limit int) {
|
||||
findings, _ := res["findings"].([]any)
|
||||
for _, f := range findings {
|
||||
fm, _ := f.(map[string]any)
|
||||
@@ -702,7 +879,6 @@ func reviewLine(r api.Review) string {
|
||||
}
|
||||
where += ")"
|
||||
}
|
||||
fmt.Fprintf(b, "\n - [%v] %v%s: %s", fm["severity"], fm["title"], where, clip(strings.TrimSpace(fmt.Sprint(fm["detail"])), 300))
|
||||
fmt.Fprintf(b, "%s- [%v] %v%s: %s", lead, fm["severity"], fm["title"], where, clip(strings.TrimSpace(fmt.Sprint(fm["detail"])), limit))
|
||||
}
|
||||
return b.String()
|
||||
}
|
||||
|
||||
@@ -28,7 +28,25 @@ func (m *Manager) SaveWorkflow(ctx context.Context, name string, file []byte, ac
|
||||
if err != nil {
|
||||
return api.WorkflowSummary{}, err
|
||||
}
|
||||
return api.WorkflowSummary{Name: name, Description: def.Description}, nil
|
||||
kind := api.WorkflowSummaryKindTask
|
||||
if def.Kind == api.WorkflowKindReview {
|
||||
kind = api.WorkflowSummaryKindReview
|
||||
}
|
||||
return api.WorkflowSummary{Name: name, Description: def.Description, Kind: kind}, nil
|
||||
}
|
||||
|
||||
func (m *Manager) SetReviewWorkflow(ctx context.Context, projectID, name, actor string) (api.Project, error) {
|
||||
name = strings.TrimSpace(name)
|
||||
if name != "" {
|
||||
def, err := m.store.GetWorkflow(ctx, name)
|
||||
if err != nil {
|
||||
return api.Project{}, err
|
||||
}
|
||||
if def.Kind != api.WorkflowKindReview {
|
||||
return api.Project{}, fmt.Errorf("%w: workflow %q is a task workflow; a project reviews with a workflow of kind review", ErrInvalid, name)
|
||||
}
|
||||
}
|
||||
return m.store.SetReviewWorkflow(ctx, projectID, name, actor)
|
||||
}
|
||||
|
||||
func (m *Manager) WorkflowFile(ctx context.Context, name string) ([]byte, error) {
|
||||
@@ -150,13 +168,13 @@ func (m *Manager) WorkflowRun(ctx context.Context, taskID string) (api.WorkflowR
|
||||
attempts[*t.WorkflowStage] = append(attempts[*t.WorkflowStage], a)
|
||||
}
|
||||
|
||||
for _, s := range frozen.Definition.Stages {
|
||||
for _, s := range workflow.Stages(frozen.Definition) {
|
||||
stage := api.WorkflowRunStage{
|
||||
Name: s.Name,
|
||||
After: workflow.After(frozen.Definition, s.Name),
|
||||
Name: s.Stage,
|
||||
After: workflow.After(frozen.Definition, s.Stage),
|
||||
State: api.StageStateWaiting,
|
||||
WaitingOn: []string{},
|
||||
Attempts: attempts[s.Name],
|
||||
Attempts: attempts[s.Stage],
|
||||
}
|
||||
if stage.Attempts == nil {
|
||||
stage.Attempts = []api.StageAttempt{}
|
||||
@@ -212,6 +230,9 @@ func (m *Manager) StartWorkflow(ctx context.Context, project api.Project, creato
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
if frozen.Definition.Kind == api.WorkflowKindReview {
|
||||
return nil, fmt.Errorf("%w: workflow %q is a review workflow, which a project names to review its snapshots; it doesn't run on a brief", ErrInvalid, name)
|
||||
}
|
||||
frozen.Brief = brief
|
||||
frozen.Title = title
|
||||
|
||||
@@ -726,6 +747,31 @@ func (m *Manager) workflowTools(tool func(name, description, schema string, run
|
||||
return fmt.Sprintf("Saved workflow %q.", w.Name), nil
|
||||
}),
|
||||
|
||||
tool("set_review_workflow",
|
||||
"Set the review workflow (a saved workflow of kind review) whose stages review every snapshot of a project's tasks, from the next snapshot on; an empty workflow turns reviews off for the project. Only when the human asks.",
|
||||
`{"type": "object", "properties": {
|
||||
"project_id": {"type": "string"},
|
||||
"workflow": {"type": "string"}
|
||||
}, "required": ["project_id", "workflow"]}`,
|
||||
func(ctx context.Context, user api.User, raw json.RawMessage) (string, error) {
|
||||
args := struct {
|
||||
ProjectID string `json:"project_id"`
|
||||
Workflow string `json:"workflow"`
|
||||
}{}
|
||||
err := json.Unmarshal(raw, &args)
|
||||
if err != nil {
|
||||
return "", err
|
||||
}
|
||||
p, err := m.SetReviewWorkflow(ctx, args.ProjectID, args.Workflow, leadActor(user.Login))
|
||||
if err != nil {
|
||||
return "", err
|
||||
}
|
||||
if p.ReviewWorkflow == "" {
|
||||
return fmt.Sprintf("Project %s has no review workflow now: its snapshots aren't reviewed.", p.Name), nil
|
||||
}
|
||||
return fmt.Sprintf("Project %s reviews with workflow %q from its next snapshot on.", p.Name, p.ReviewWorkflow), nil
|
||||
}),
|
||||
|
||||
tool("delete_workflow",
|
||||
"Delete a workflow from the server. Tasks that froze it keep their copy. Only when the human asks.",
|
||||
`{"type": "object", "properties": {"name": {"type": "string"}}, "required": ["name"]}`,
|
||||
|
||||
Reference in New Issue
Block a user