orchestra/internal/review/review.go

// Package review holds independent review state: the verified evidence a
// reviewer is given, and the bounded findings it returns.
//
// Independence is structural, not a request. The reviewer receives the diff,
// the contract, the decisions, and the accepted plan. It does not receive the
// implementation's transcript, handoff, or completion claims, so it has to
// reconstruct whether the diff satisfies the contract instead of agreeing with
// whoever wrote it.
package review

import (
	"encoding/json"
	"fmt"
	"strings"
)

type Severity string

const (
	// Blocker and Important both send the work back. Minor is reported and
	// left to judgement.
	//
	// There is deliberately no "invalid" severity. Whether a finding was
	// wrong is a conclusion the implementer or an operator reaches later, not
	// something a reviewer can report about its own output.
	Blocker   Severity = "blocker"
	Important Severity = "important"
	Minor     Severity = "minor"
)

func (s Severity) Valid() bool {
	switch s {
	case Blocker, Important, Minor:
		return true
	}
	return false
}

// Blocking reports whether this severity returns the task to implementation.
func (s Severity) Blocking() bool { return s == Blocker || s == Important }

type Finding struct {
	ID       string   `json:"id"`
	Severity Severity `json:"severity"`
	File     string   `json:"file"`
	Line     int      `json:"line,omitempty"`
	Claim    string   `json:"claim"`
	Evidence string   `json:"evidence"`
}

// Result is one review, bound to the exact commit it was performed against.
// A review is never a free-floating boolean: if the code moves, the review
// describes a tree that no longer exists.
type Result struct {
	ResultSHA string    `json:"result_sha"`
	Findings  []Finding `json:"findings"`
}

// Evidence is what the reviewer is given about the change itself. Every field
// is verified by Orchestra rather than reported by the implementer.
type Evidence struct {
	BaseSHA     string `json:"base_sha"`
	ResultSHA   string `json:"result_sha"`
	Diff        string `json:"diff"`
	GateCommand string `json:"gate_command,omitempty"`
	GateExit    int    `json:"gate_exit"`
	GateOutput  string `json:"gate_output,omitempty"`
}

const (
	maxFindings = 40
	maxField    = 500
	// MaxDiffBytes bounds what reaches a context window. A change too large to
	// render is a change too large to review in one session.
	MaxDiffBytes = 256 << 10
	// MaxGateOutputBytes keeps a failing gate's log from crowding out the diff.
	MaxGateOutputBytes = 8 << 10
)

func (r Result) Validate() error {
	if len(r.ResultSHA) != 40 {
		return fmt.Errorf("review: result_sha must be a full commit sha")
	}
	if len(r.Findings) > maxFindings {
		return fmt.Errorf("review: %d findings exceeds the %d bound", len(r.Findings), maxFindings)
	}
	seen := map[string]bool{}
	for i, f := range r.Findings {
		if strings.TrimSpace(f.ID) == "" {
			return fmt.Errorf("review: findings[%d].id is required", i)
		}
		if seen[f.ID] {
			return fmt.Errorf("review: duplicate finding id %q", f.ID)
		}
		seen[f.ID] = true
		if !f.Severity.Valid() {
			return fmt.Errorf("review: findings[%d].severity %q is not blocker, important, or minor", i, f.Severity)
		}
		if err := field(fmt.Sprintf("findings[%d].file", i), f.File, true); err != nil {
			return err
		}
		if strings.HasPrefix(f.File, "/") {
			return fmt.Errorf("review: findings[%d].file must be repository-relative", i)
		}
		if f.Line < 0 {
			return fmt.Errorf("review: findings[%d].line cannot be negative", i)
		}
		if err := field(fmt.Sprintf("findings[%d].claim", i), f.Claim, true); err != nil {
			return err
		}
		if err := field(fmt.Sprintf("findings[%d].evidence", i), f.Evidence, true); err != nil {
			return err
		}
	}
	return nil
}

// Blocking returns the findings that send the work back.
func (r Result) Blocking() []Finding {
	var out []Finding
	for _, f := range r.Findings {
		if f.Severity.Blocking() {
			out = append(out, f)
		}
	}
	return out
}

// Accepted reports whether this review lets the task proceed. Minor findings
// are reported and left to judgement rather than forced.
func (r Result) Accepted() bool { return len(r.Blocking()) == 0 }

func field(name, v string, required bool) error {
	s := strings.TrimSpace(v)
	if s == "" {
		if required {
			return fmt.Errorf("review: %s is required", name)
		}
		return nil
	}
	if len(s) > maxField {
		return fmt.Errorf("review: %s exceeds %d characters", name, maxField)
	}
	if strings.ContainsAny(s, "\n\r") {
		return fmt.Errorf("review: %s must be a single line", name)
	}
	return nil
}

func Encode(r Result) ([]byte, error) {
	if err := r.Validate(); err != nil {
		return nil, err
	}
	return json.Marshal(r)
}

func Decode(b []byte) (Result, error) {
	var r Result
	if err := json.Unmarshal(b, &r); err != nil {
		return Result{}, fmt.Errorf("review artifact: %w", err)
	}
	return r, r.Validate()
}

// Instructions is the reviewer's whole brief. It is narrow on purpose: an open
// invitation produces a list of ways the reviewer would have written it
// instead, which is not review.
const Instructions = `Review the supplied diff against, in order:
1. the task goal and acceptance criteria
2. the human decisions and constraints
3. the repository rules
4. the accepted plan
5. observable correctness and regressions

Report only concrete findings supported by the diff or by repository evidence
you can point at. Every finding needs a file, a claim, and the evidence for it.

Severity: blocker if it is wrong or unsafe, important if it will cause a real
defect or contradicts a decision, minor otherwise.

Do not redesign the solution. Do not suggest optional refactors. Do not edit
any file. Do not report style preferences unless they violate a repository
rule. You are not implementing this task and you do not decide its lifecycle.`