feat(pipeline): persist uncertified fixer commits for the next initial review (#745)

* feat(review): persist uncertified fixer commits for the next initial review

A cancelled re-review left pipeline-authored commits with no marker, so the replacement run reviewed them as ordinary author code. Persist the per-branch uncertified range and feed fix-round provenance on the next initial review; rerun still proceeds.

* no-mistakes(review): Preserve uncertified range across runs and rebases

* no-mistakes(document): Document uncertified review provenance across runs
This commit is contained in:
Kun Chen
2026-08-15 02:33:01 -07:00
committed by GitHub
parent f92b7bd6da
commit a68298ea66
17 changed files with 1255 additions and 8 deletions
+7 -1
View File
@@ -150,11 +150,17 @@ Safest local verification sequence after non-trivial changes:
**Review-Loop Agent Sessions (`internal/pipeline/sessions.go`)**
- Per run, the review loop keeps ONE durable fixer session across review-fix turns, and EVERY review turn (initial review and every full rereview) runs session-free. A rereview certifies fixes implementing the previous review turn's findings, so resuming any review session seats the prescriber as certifier - the mechanism that let one fix round ship wrong code plus the test blessing it with zero findings. Cross-round review context travels only in the explicit sanitized round history; the fixer session is never lent to review turns, no other step uses sessions, and sessions are keyed strictly by run. The rereview prompt reframes fix-round changes as pipeline-authored code under the author-grade adversarial standard (`fixRoundProvenanceClause`); prior findings, fix summaries, and same-round tests are claims, not evidence.
- Per run, the review loop keeps ONE durable fixer session across review-fix turns, and EVERY review turn (initial review and every full rereview) runs session-free. A rereview certifies fixes implementing the previous review turn's findings, so resuming any review session seats the prescriber as certifier - the mechanism that let one fix round ship wrong code plus the test blessing it with zero findings. Cross-round review context travels only in the explicit sanitized round history; the fixer session is never lent to review turns, no other step uses sessions, and sessions are keyed strictly by run. The rereview prompt reframes fix-round changes as pipeline-authored code under the author-grade adversarial standard (`fixRoundProvenanceClause`); the same clause is emitted on a later run's initial review when a persisted uncertified range is bound. Prior findings, fix summaries, and same-round tests are claims, not evidence.
- Fail-safe rules: unsupported adapter runs cold; a failed fixer resume drops the identity and re-runs the same turn in a fresh fixer session, never skipping the turn; a cancelled ctx gets no fallback retry; `session_reuse: false` forces everything cold. Persistence is minimum metadata only, never prompts or transcripts; `SessionRoleReviewer` remains only so crash recovery accepts legacy persisted rows, which are never resumed.
- `codex exec resume` has a narrower flag surface than `codex exec`, so an unsupported override fails the resume and falls back; the e2e fakeagent must keep parsing both codex argv shapes (`extractCodexPrompt`).
- Regressions: `internal/pipeline/sessions_test.go`, `internal/pipeline/steps/review_session_test.go` (incl. `TestReviewLoop_RereviewNeverResumesTheSessionThatPrescribedItsFixes`), `TestReviewStep_RereviewTreatsFixRoundsAsPipelineAuthoredCode`, `internal/agent/session_test.go`.
**Uncertified Review Provenance (`internal/pipeline/uncertified.go`)**
- When a review-step fixer round commits and its re-review does not complete, persist the per-branch uncertified range (`from_sha`, `to_sha`). Persist on review-step fixer commits only, not lint or document. On the next run's initial review, bind that range and emit `fixRoundProvenanceClause` even when `Fixing==false`, so the replacement reviewer is not cold. Rerun proceeds; there is no refusal or `--ack-uncertified-review` gate.
- Missing git objects warn and continue, never block. Clear the range only after a completed review whose approved head equals or is a descendant of `to_sha`; parked, failed, skipped, and aborted reviews must not clear it. Rebase remaps the persisted SHAs onto the rewritten head so the next review can still bind.
- Regressions: `internal/pipeline/uncertified_test.go`, `TestCommitAgentFixes_PersistsUncertifiedRangeForReview`, `TestCommitAgentFixes_LintDoesNotPersistUncertifiedRange`, `TestCommitAgentFixes_DocumentDoesNotPersistUncertifiedRange`, `TestFixRoundProvenanceClause_EmitsForUncertifiedRangeWhenNotFixing`, `TestUncertifiedRange_PersistsThenFeedsNextInitialReview`, `TestRebaseStep_RemapsUncertifiedRangeWhenHeadRewritten`.
**Review Fixer Verification Discipline (`internal/pipeline/steps/review.go`)**
- The review-fix prompt requires all fixes before one focused verification limited to the changed area and forbids the whole repository test/lint suite during the fix round.
@@ -52,6 +52,7 @@ The pipeline is opinionated so that "passed the gate" has a stable meaning:
It also stops when the branch would silently bundle commits from a local default branch that were never pushed to `origin/<default_branch>`.
If there's no diff left after the rebase, the pipeline skips the rest.
- **Review before test** so the agent reads fresh code, not code it may have touched during fixes.
A later run's initial review also receives fix-round provenance for any uncertified pipeline-authored commits left on the branch when a previous run's re-review did not complete.
- **Document after test** so docs are updated against code that's known to work.
- **Lint last among local checks** so it doesn't churn over code that may still change.
- **Push → PR → CI** happens after all local checks pass.
@@ -83,6 +83,7 @@ AI code review of your diff.
- Agent returns findings with severity (`error`, `warning`, `info`), file location, description, and an `action` (`no-op`, `auto-fix`, `ask-user`)
- Also returns a `risk_level` (`low`, `medium`, `high`) and `risk_rationale`
- Runs every review turn - the initial review and every full rereview - as a fresh, session-free invocation, so the rereview that certifies a fix round never resumes the session whose findings prescribed those fixes; the rereview prompt additionally reframes fix-round changes as pipeline-authored code to review under the same adversarial standard as the author's changes, with prior findings, fix summaries, and same-round tests treated as claims rather than evidence
- When a review-step fixer round commits and its re-review does not complete, persists that branch's uncertified commit range (lint and document fixer commits do not); the next run's initial review of that range receives the same pipeline-authored provenance framing so the replacement reviewer is not cold. A later rebase remaps the persisted SHAs onto the rewritten head. The range is cleared only after a completed review whose approved head equals or descends from the range tip; parked, failed, skipped, and aborted reviews leave it in place
- With the default `session_reuse: true`, Claude and Codex reuse one durable fixer session across review-fix turns; a resume failure retries the same fix turn in a fresh fixer session, and unsupported agents run cold
- Atomically records the exact commit examined when a full review completes successfully; a parked review retains its candidate only for recovery, while failed, skipped, superseded, and legacy reviews grant no inferred approval authority
+14
View File
@@ -139,6 +139,20 @@ CREATE TABLE IF NOT EXISTS intent_cache (
session_id TEXT NOT NULL,
created_at INTEGER NOT NULL
);
-- Per-branch range of pipeline-authored commits whose re-review did not
-- complete. The next run's initial review reads this so it is not cold on
-- uncertified fixer commits. PRIMARY KEY per branch: the latest uncertified
-- HEAD replaces an older range.
CREATE TABLE IF NOT EXISTS uncertified_pipeline_ranges (
repo_id TEXT NOT NULL REFERENCES repos(id) ON DELETE CASCADE,
branch TEXT NOT NULL,
from_sha TEXT NOT NULL,
to_sha TEXT NOT NULL,
source_run_id TEXT NOT NULL,
created_at INTEGER NOT NULL,
PRIMARY KEY (repo_id, branch)
);
`
// migrationStatements hold additive schema changes applied to databases that
+87
View File
@@ -0,0 +1,87 @@
package db
import (
"database/sql"
"fmt"
"strings"
)
// UncertifiedPipelineRange is the per-branch span of pipeline-authored
// commits whose re-review did not complete. The next run on that branch
// feeds this range into the initial review so the replacement reviewer is
// not cold. The database range is the authority; commit messages are not.
type UncertifiedPipelineRange struct {
RepoID string
Branch string
FromSHA string
ToSHA string
SourceRunID string
CreatedAt int64
}
// UpsertUncertifiedPipelineRange records or replaces the uncertified fixer
// range for one repo+branch. A newer uncertified HEAD replaces an older one.
func (d *DB) UpsertUncertifiedPipelineRange(repoID, branch, fromSHA, toSHA, sourceRunID string) error {
repoID = strings.TrimSpace(repoID)
branch = strings.TrimSpace(branch)
fromSHA = strings.TrimSpace(fromSHA)
toSHA = strings.TrimSpace(toSHA)
sourceRunID = strings.TrimSpace(sourceRunID)
if repoID == "" || branch == "" || fromSHA == "" || toSHA == "" || sourceRunID == "" {
return fmt.Errorf("uncertified pipeline range requires repo, branch, from_sha, to_sha, and source run")
}
_, err := d.sql.Exec(
`INSERT INTO uncertified_pipeline_ranges (repo_id, branch, from_sha, to_sha, source_run_id, created_at)
VALUES (?, ?, ?, ?, ?, ?)
ON CONFLICT(repo_id, branch) DO UPDATE SET
from_sha = excluded.from_sha,
to_sha = excluded.to_sha,
source_run_id = excluded.source_run_id,
created_at = excluded.created_at`,
repoID, branch, fromSHA, toSHA, sourceRunID, now(),
)
if err != nil {
return fmt.Errorf("upsert uncertified pipeline range: %w", err)
}
return nil
}
// GetUncertifiedPipelineRange returns the uncertified range for a branch, or
// nil when none is recorded.
func (d *DB) GetUncertifiedPipelineRange(repoID, branch string) (*UncertifiedPipelineRange, error) {
repoID = strings.TrimSpace(repoID)
branch = strings.TrimSpace(branch)
if repoID == "" || branch == "" {
return nil, nil
}
row := d.sql.QueryRow(
`SELECT repo_id, branch, from_sha, to_sha, source_run_id, created_at
FROM uncertified_pipeline_ranges WHERE repo_id = ? AND branch = ?`,
repoID, branch,
)
var r UncertifiedPipelineRange
if err := row.Scan(&r.RepoID, &r.Branch, &r.FromSHA, &r.ToSHA, &r.SourceRunID, &r.CreatedAt); err != nil {
if err == sql.ErrNoRows {
return nil, nil
}
return nil, fmt.Errorf("get uncertified pipeline range: %w", err)
}
return &r, nil
}
// DeleteUncertifiedPipelineRange removes the uncertified marker for a branch.
// It is a no-op when no row exists.
func (d *DB) DeleteUncertifiedPipelineRange(repoID, branch string) error {
repoID = strings.TrimSpace(repoID)
branch = strings.TrimSpace(branch)
if repoID == "" || branch == "" {
return nil
}
if _, err := d.sql.Exec(
`DELETE FROM uncertified_pipeline_ranges WHERE repo_id = ? AND branch = ?`,
repoID, branch,
); err != nil {
return fmt.Errorf("delete uncertified pipeline range: %w", err)
}
return nil
}
+76
View File
@@ -0,0 +1,76 @@
package db
import "testing"
func TestUncertifiedPipelineRangeUpsertGetDelete(t *testing.T) {
d := openTestDB(t)
repo, err := d.InsertRepo("/work/repo", "https://example.com/repo.git", "main")
if err != nil {
t.Fatal(err)
}
run, err := d.InsertRun(repo.ID, "feature", "head-1", "base")
if err != nil {
t.Fatal(err)
}
got, err := d.GetUncertifiedPipelineRange(repo.ID, "feature")
if err != nil {
t.Fatal(err)
}
if got != nil {
t.Fatalf("empty lookup = %#v, want nil", got)
}
if err := d.UpsertUncertifiedPipelineRange(repo.ID, "feature", "from-a", "to-a", run.ID); err != nil {
t.Fatal(err)
}
got, err = d.GetUncertifiedPipelineRange(repo.ID, "feature")
if err != nil {
t.Fatal(err)
}
if got == nil || got.FromSHA != "from-a" || got.ToSHA != "to-a" || got.SourceRunID != run.ID {
t.Fatalf("first upsert = %#v", got)
}
run2, err := d.InsertRun(repo.ID, "feature", "head-2", "base")
if err != nil {
t.Fatal(err)
}
if err := d.UpsertUncertifiedPipelineRange(repo.ID, "feature", "from-b", "to-b", run2.ID); err != nil {
t.Fatal(err)
}
got, err = d.GetUncertifiedPipelineRange(repo.ID, "feature")
if err != nil {
t.Fatal(err)
}
if got == nil || got.FromSHA != "from-b" || got.ToSHA != "to-b" || got.SourceRunID != run2.ID {
t.Fatalf("replacement upsert = %#v, want latest range only", got)
}
other, err := d.GetUncertifiedPipelineRange(repo.ID, "other")
if err != nil {
t.Fatal(err)
}
if other != nil {
t.Fatalf("other branch = %#v, want nil", other)
}
if err := d.DeleteUncertifiedPipelineRange(repo.ID, "feature"); err != nil {
t.Fatal(err)
}
got, err = d.GetUncertifiedPipelineRange(repo.ID, "feature")
if err != nil {
t.Fatal(err)
}
if got != nil {
t.Fatalf("after delete = %#v, want nil", got)
}
}
func TestOpenCreatesUncertifiedPipelineRangesTable(t *testing.T) {
d := openTestDB(t)
var count int
if err := d.sql.QueryRow("SELECT count(*) FROM uncertified_pipeline_ranges").Scan(&count); err != nil {
t.Fatalf("uncertified_pipeline_ranges table missing: %v", err)
}
}
+5
View File
@@ -306,6 +306,7 @@ func (e *Executor) Resume(ctx context.Context, run *db.Run, repo *db.Repo, workD
}
reviewedHead := gate.reviewedHeadSHA
run.ReviewApprovedHeadSHA = &reviewedHead
ClearUncertifiedPipelineRangeIfCertified(ctx, e.db, repo.ID, run.Branch, reviewedHead, workDir)
return nil
}
return e.db.CompleteStepWithStatus(gate.stepResult.ID, types.StepStatusCompleted, recoveredExitCode(gate.stepResult), duration, recoveredLogPath(gate.stepResult))
@@ -739,6 +740,9 @@ func (e *Executor) executeStep(ctx context.Context, step Step, sr *db.StepResult
CIReadinessChanged: ciReadinessChanged,
OnPRMerged: e.onPRMerged,
}
if stepName == types.StepReview {
BindUncertifiedPipelineRange(sctx)
}
nextTrigger := "initial"
if sctx.Fixing {
@@ -1009,6 +1013,7 @@ done:
}
reviewedHead := reviewApprovedHeadSHA
run.ReviewApprovedHeadSHA = &reviewedHead
ClearUncertifiedPipelineRangeIfCertified(ctx, e.db, repo.ID, run.Branch, reviewedHead, workDir)
} else if err := e.db.CompleteStepWithStatus(sr.ID, status, finalExitCode, durationMS, logPath); err != nil {
return false, fmt.Errorf("complete step %s: %w", stepName, err)
}
+10
View File
@@ -51,6 +51,16 @@ type StepContext struct {
// authoritative acceptance criteria; an agent name ("claude", "codex", ...)
// means it was inferred from a transcript (a hint). Empty when no intent exists.
IntentSource string
// UncertifiedFromSHA/ToSHA/SourceRunID name a previous run's fixer
// commits on this branch whose re-review did not complete. They are set
// on a later run's initial review (Fixing==false) so that review still
// receives fix-round provenance. Empty when no such range applies.
UncertifiedFromSHA string
UncertifiedToSHA string
UncertifiedSourceRunID string
// UncertifiedPriorRounds are review rounds from the source run that left
// the uncertified range. Nil when none apply.
UncertifiedPriorRounds []*db.StepRound
// Sessions manages the run's durable review-fixer session. The session
// machinery remains role-generic for legacy recovery; nil runs every
// invocation cold.
+7
View File
@@ -152,10 +152,17 @@ func commitAgentFixes(sctx *pipeline.StepContext, stepName types.StepName, summa
if _, err := git.Run(ctx, sctx.WorkDir, "update-ref", ref, headSHA); err != nil {
return fmt.Errorf("update local branch ref: %w", err)
}
startingHead := strings.TrimSpace(sctx.ReviewStartingHeadSHA)
if startingHead == "" {
startingHead = sctx.Run.HeadSHA
}
sctx.Run.HeadSHA = headSHA
if err := sctx.DB.UpdateRunHeadSHA(sctx.Run.ID, headSHA); err != nil {
return err
}
if stepName == types.StepReview {
pipeline.PersistUncertifiedPipelineRange(sctx, startingHead, headSHA)
}
sctx.Log(fmt.Sprintf("committed agent fixes: %s", commitMessage))
return nil
}
+72
View File
@@ -578,6 +578,78 @@ func TestStepCmd_OverridesPathWithoutDuplicateEntries(t *testing.T) {
}
}
func TestCommitAgentFixes_PersistsUncertifiedRangeForReview(t *testing.T) {
t.Parallel()
dir, baseSHA, headSHA := setupGitRepo(t)
gitCmd(t, dir, "checkout", "--detach", headSHA)
ag := &mockAgent{name: "test"}
sctx := newTestContextWithDBRecords(t, ag, dir, baseSHA, headSHA, config.Commands{})
sctx.ReviewStartingHeadSHA = headSHA
if err := os.WriteFile(filepath.Join(dir, "review-fix.txt"), []byte("fixed"), 0o644); err != nil {
t.Fatal(err)
}
if err := commitAgentFixes(sctx, types.StepReview, "apply fix", "fallback"); err != nil {
t.Fatal(err)
}
got, err := sctx.DB.GetUncertifiedPipelineRange(sctx.Repo.ID, sctx.Run.Branch)
if err != nil {
t.Fatal(err)
}
if got == nil || got.FromSHA != headSHA || got.ToSHA != sctx.Run.HeadSHA || got.SourceRunID != sctx.Run.ID {
t.Fatalf("uncertified range = %#v, want from=%s to=%s run=%s", got, headSHA, sctx.Run.HeadSHA, sctx.Run.ID)
}
if got.FromSHA == got.ToSHA {
t.Fatal("uncertified range did not advance HEAD")
}
}
func TestCommitAgentFixes_LintDoesNotPersistUncertifiedRange(t *testing.T) {
t.Parallel()
dir, baseSHA, headSHA := setupGitRepo(t)
gitCmd(t, dir, "checkout", "--detach", headSHA)
ag := &mockAgent{name: "test"}
sctx := newTestContextWithDBRecords(t, ag, dir, baseSHA, headSHA, config.Commands{})
sctx.ReviewStartingHeadSHA = headSHA
if err := os.WriteFile(filepath.Join(dir, "lint-fix.txt"), []byte("fixed"), 0o644); err != nil {
t.Fatal(err)
}
if err := commitAgentFixes(sctx, types.StepLint, "apply fix", "fallback"); err != nil {
t.Fatal(err)
}
got, err := sctx.DB.GetUncertifiedPipelineRange(sctx.Repo.ID, sctx.Run.Branch)
if err != nil {
t.Fatal(err)
}
if got != nil {
t.Fatalf("lint persist = %#v, want no uncertified range", got)
}
}
func TestCommitAgentFixes_DocumentDoesNotPersistUncertifiedRange(t *testing.T) {
t.Parallel()
dir, baseSHA, headSHA := setupGitRepo(t)
gitCmd(t, dir, "checkout", "--detach", headSHA)
ag := &mockAgent{name: "test"}
sctx := newTestContextWithDBRecords(t, ag, dir, baseSHA, headSHA, config.Commands{})
sctx.ReviewStartingHeadSHA = headSHA
if err := os.WriteFile(filepath.Join(dir, "docs-fix.txt"), []byte("fixed"), 0o644); err != nil {
t.Fatal(err)
}
if err := commitAgentFixes(sctx, types.StepDocument, "apply fix", "fallback"); err != nil {
t.Fatal(err)
}
got, err := sctx.DB.GetUncertifiedPipelineRange(sctx.Repo.ID, sctx.Run.Branch)
if err != nil {
t.Fatal(err)
}
if got != nil {
t.Fatalf("document persist = %#v, want no uncertified range", got)
}
}
func TestCommitAgentFixes_NoChanges(t *testing.T) {
t.Parallel()
dir, baseSHA, headSHA := setupGitRepo(t)
+2
View File
@@ -505,6 +505,8 @@ func updateHeadSHA(ctx context.Context, sctx *pipeline.StepContext) (*pipeline.S
return nil, fmt.Errorf("resolve head after rebase: %w", err)
}
if headSHA != "" && headSHA != sctx.Run.HeadSHA {
oldHead := sctx.Run.HeadSHA
pipeline.RemapUncertifiedPipelineRangeAfterRebase(sctx, oldHead, headSHA)
sctx.Run.HeadSHA = headSHA
if err := sctx.DB.UpdateRunHeadSHA(sctx.Run.ID, headSHA); err != nil {
return nil, err
+76
View File
@@ -12,6 +12,7 @@ import (
"github.com/kunchenguid/no-mistakes/internal/agent"
"github.com/kunchenguid/no-mistakes/internal/config"
"github.com/kunchenguid/no-mistakes/internal/pipeline"
)
func TestRebaseStep_ConflictTriesAllTargets(t *testing.T) {
@@ -419,3 +420,78 @@ func TestRebaseStep_LogFileNotVisibleToUser(t *testing.T) {
t.Error("expected fetch warning in file logs")
}
}
func TestRebaseStep_RemapsUncertifiedRangeWhenHeadRewritten(t *testing.T) {
t.Parallel()
upstream := t.TempDir()
gitCmd(t, upstream, "init", "--bare")
dir := t.TempDir()
gitCmd(t, dir, "init")
gitCmd(t, dir, "config", "user.name", "test")
gitCmd(t, dir, "config", "user.email", "test@test.com")
gitCmd(t, dir, "checkout", "-b", "main")
gitCmd(t, dir, "remote", "add", "origin", upstream)
os.WriteFile(filepath.Join(dir, "base.txt"), []byte("base\n"), 0o644)
gitCmd(t, dir, "add", "-A")
gitCmd(t, dir, "commit", "-m", "base commit")
baseSHA := gitCmd(t, dir, "rev-parse", "HEAD")
gitCmd(t, dir, "push", "origin", "main")
gitCmd(t, dir, "checkout", "-b", "feature")
os.WriteFile(filepath.Join(dir, "author.txt"), []byte("author\n"), 0o644)
gitCmd(t, dir, "add", "-A")
gitCmd(t, dir, "commit", "-m", "author")
fromSHA := gitCmd(t, dir, "rev-parse", "HEAD")
os.WriteFile(filepath.Join(dir, "fixer.txt"), []byte("fixer\n"), 0o644)
gitCmd(t, dir, "add", "-A")
gitCmd(t, dir, "commit", "-m", "fixer")
toSHA := gitCmd(t, dir, "rev-parse", "HEAD")
gitCmd(t, dir, "push", "origin", "feature")
gitCmd(t, dir, "checkout", "main")
os.WriteFile(filepath.Join(dir, "other.txt"), []byte("main update\n"), 0o644)
gitCmd(t, dir, "add", "-A")
gitCmd(t, dir, "commit", "-m", "main non-conflicting update")
gitCmd(t, dir, "push", "origin", "main")
gitCmd(t, dir, "checkout", "feature")
ag := &mockAgent{name: "test"}
sctx := newTestContextWithDBRecords(t, ag, dir, baseSHA, toSHA, config.Commands{})
sctx.Run.Branch = "refs/heads/feature"
sctx.Repo.UpstreamURL = upstream
if err := sctx.DB.UpsertUncertifiedPipelineRange(sctx.Repo.ID, sctx.Run.Branch, fromSHA, toSHA, sctx.Run.ID); err != nil {
t.Fatal(err)
}
if _, err := (&RebaseStep{}).Execute(sctx); err != nil {
t.Fatal(err)
}
newHead := gitCmd(t, dir, "rev-parse", "HEAD")
if newHead == toSHA {
t.Fatal("rebase did not rewrite the uncertified head")
}
got, err := sctx.DB.GetUncertifiedPipelineRange(sctx.Repo.ID, sctx.Run.Branch)
if err != nil {
t.Fatal(err)
}
if got == nil || got.FromSHA == fromSHA || got.ToSHA == toSHA {
t.Fatalf("updateHeadSHA left old uncertified SHAs: %#v (old from=%s to=%s)", got, fromSHA, toSHA)
}
if got.ToSHA != newHead || got.SourceRunID != sctx.Run.ID {
t.Fatalf("remapped range = %#v, want to=%s source=%s", got, newHead, sctx.Run.ID)
}
bind := &pipeline.StepContext{
Ctx: sctx.Ctx,
DB: sctx.DB,
Repo: sctx.Repo,
Run: sctx.Run,
WorkDir: dir,
}
pipeline.BindUncertifiedPipelineRange(bind)
if bind.UncertifiedFromSHA != got.FromSHA || bind.UncertifiedToSHA != got.ToSHA {
t.Fatalf("bind after rebase remap from=%q to=%q, want from=%q to=%q", bind.UncertifiedFromSHA, bind.UncertifiedToSHA, got.FromSHA, got.ToSHA)
}
}
+23 -7
View File
@@ -165,7 +165,7 @@ Previous review findings to address:
// net-deleted-author-lines git-diff backstop for the removal-of-required
// class - a fixer round that net-deletes author-added lines parks
// regardless of intent source. Held pending a scope decision.
historySection := executionContextPromptSection() + roundHistoryPromptSection(sctx) + fixRoundProvenanceClause(sctx) + userIntentPromptSection(sctx) + intentConformanceReviewClause(sctx) + pipelineDeliveryPhaseClause() + testguidance.Rule + testguidance.ReviewerAction
historySection := executionContextPromptSection() + roundHistoryPromptSection(sctx) + uncertifiedRoundHistoryPromptSection(sctx) + fixRoundProvenanceClause(sctx) + userIntentPromptSection(sctx) + intentConformanceReviewClause(sctx) + pipelineDeliveryPhaseClause() + testguidance.Rule + testguidance.ReviewerAction
// Path-scoped repository review guidance, taken from the trusted
// default-branch config copy (regardless of allow_repo_commands) so a pushed
@@ -296,13 +296,15 @@ Risk assessment (after listing all findings):
// it, the round-history section reads as "found and fixed" and invites less
// scrutiny of exactly the code the pipeline itself just wrote: the fixer
// authors both code and tests in one round, so the only independent check
// that code ever gets is this rereview. Empty outside fix mode, leaving the
// initial review prompt unchanged.
// that code ever gets is this rereview.
//
// The same framing is emitted for an uncertified range left by a previous
// run whose re-review did not complete, even when Fixing is false, so a
// replacement initial review is not cold on those commits. Empty when
// neither case applies, leaving an ordinary initial review unchanged.
func fixRoundProvenanceClause(sctx *pipeline.StepContext) string {
if !sctx.Fixing {
return ""
}
return `
if sctx != nil && sctx.Fixing {
return `
Fix-round provenance:
- This is a re-review after this run's automated fix round(s): every commit after the starting head, plus any uncommitted worktree changes, was authored by the pipeline's own fixer agent, not by the change author.
@@ -310,6 +312,20 @@ Fix-round provenance:
- Prior findings and fix summaries are claims, not evidence. Verify each claimed fix against the current code, and independently judge whether behavior the fix rounds introduced is correct, not merely whether it implements what was prescribed.
- A test added or changed in the same fix round as the code it exercises is part of that round's claim, not independent proof: judge whether its asserted outcome is the right outcome and whether it could still pass with the code wrong.
`
}
if sctx == nil || strings.TrimSpace(sctx.UncertifiedToSHA) == "" {
return ""
}
fromSHA := strings.TrimSpace(sctx.UncertifiedFromSHA)
toSHA := strings.TrimSpace(sctx.UncertifiedToSHA)
return fmt.Sprintf(`
Fix-round provenance:
- Commits after %s through %s on this branch were authored by a previous run's fixer and were never certified: that run's re-review did not complete. Review them as pipeline-authored code under the same adversarial standard.
- Review that pipeline-authored code with exactly the same adversarial standard as the author's original changes. It is unreviewed new code, not a settled resolution of the findings that prompted it.
- Prior findings and fix summaries are claims, not evidence. Verify each claimed fix against the current code, and independently judge whether behavior the fix rounds introduced is correct, not merely whether it implements what was prescribed.
- A test added or changed in the same fix round as the code it exercises is part of that round's claim, not independent proof: judge whether its asserted outcome is the right outcome and whether it could still pass with the code wrong.
`, fromSHA, toSHA)
}
// approvedReviewOutcome captures the immutable commit examined by this full
+108
View File
@@ -453,6 +453,114 @@ func TestReviewStep_RereviewTreatsFixRoundsAsPipelineAuthoredCode(t *testing.T)
})
}
func TestFixRoundProvenanceClause_EmitsForUncertifiedRangeWhenNotFixing(t *testing.T) {
t.Parallel()
dir, baseSHA, headSHA := setupGitRepo(t)
findingsJSON, _ := json.Marshal(Findings{Summary: "clean"})
ag := &mockAgent{
name: "test",
runFn: func(ctx context.Context, opts agent.RunOpts) (*agent.Result, error) {
return &agent.Result{Output: findingsJSON}, nil
},
}
sctx := newTestContextWithDBRecords(t, ag, dir, baseSHA, headSHA, config.Commands{})
sctx.UncertifiedFromSHA = "from-sha"
sctx.UncertifiedToSHA = "to-sha"
sctx.UncertifiedSourceRunID = "prior-run"
priorFindings := `{"findings":[{"id":"review-1","severity":"error","file":"main.go","line":4,"description":"reachable bug","action":"auto-fix"}]}`
sctx.UncertifiedPriorRounds = []*db.StepRound{{
Round: 1,
Trigger: "initial",
FindingsJSON: &priorFindings,
}}
if _, err := (&ReviewStep{}).Execute(sctx); err != nil {
t.Fatal(err)
}
if len(ag.calls) != 1 {
t.Fatalf("expected 1 review call, got %d", len(ag.calls))
}
prompt := ag.calls[0].Prompt
for _, want := range []string{
"Fix-round provenance:",
"Commits after from-sha through to-sha on this branch were authored by a previous run's fixer and were never certified",
"same adversarial standard",
"Prior findings and fix summaries are claims, not evidence",
"Previous run (uncertified fixer commits)",
"reachable bug",
} {
if !strings.Contains(prompt, want) {
t.Errorf("initial review missing uncertified provenance %q:\n%s", want, prompt)
}
}
if strings.Contains(prompt, "This is a re-review after this run's automated fix round(s)") {
t.Errorf("uncertified initial review must not use the current-run fixer framing:\n%s", prompt)
}
}
func TestUncertifiedRange_PersistsThenFeedsNextInitialReview(t *testing.T) {
t.Parallel()
dir, baseSHA, headSHA := setupGitRepo(t)
gitCmd(t, dir, "checkout", "--detach", headSHA)
fixAgent := &mockAgent{name: "test"}
fixCtx := newTestContextWithDBRecords(t, fixAgent, dir, baseSHA, headSHA, config.Commands{})
fixCtx.ReviewStartingHeadSHA = headSHA
if err := os.WriteFile(filepath.Join(dir, "review-fix.txt"), []byte("fixed"), 0o644); err != nil {
t.Fatal(err)
}
if err := commitAgentFixes(fixCtx, types.StepReview, "apply fix", "fallback"); err != nil {
t.Fatal(err)
}
persisted, err := fixCtx.DB.GetUncertifiedPipelineRange(fixCtx.Repo.ID, fixCtx.Run.Branch)
if err != nil {
t.Fatal(err)
}
if persisted == nil || persisted.FromSHA != headSHA || persisted.ToSHA != fixCtx.Run.HeadSHA {
t.Fatalf("fixer commit did not persist range: %#v", persisted)
}
findingsJSON, _ := json.Marshal(Findings{Summary: "clean"})
reviewAgent := &mockAgent{
name: "test",
runFn: func(ctx context.Context, opts agent.RunOpts) (*agent.Result, error) {
return &agent.Result{Output: findingsJSON}, nil
},
}
nextRun, err := fixCtx.DB.InsertRun(fixCtx.Repo.ID, fixCtx.Run.Branch, fixCtx.Run.HeadSHA, baseSHA)
if err != nil {
t.Fatal(err)
}
sctx := newTestContext(t, reviewAgent, dir, baseSHA, fixCtx.Run.HeadSHA, config.Commands{})
sctx.DB = fixCtx.DB
sctx.Repo = fixCtx.Repo
sctx.Run = nextRun
sctx.Fixing = false
pipeline.BindUncertifiedPipelineRange(sctx)
if sctx.UncertifiedFromSHA != persisted.FromSHA || sctx.UncertifiedToSHA != persisted.ToSHA {
t.Fatalf("next initial review bound from=%q to=%q, want from=%q to=%q", sctx.UncertifiedFromSHA, sctx.UncertifiedToSHA, persisted.FromSHA, persisted.ToSHA)
}
if _, err := (&ReviewStep{}).Execute(sctx); err != nil {
t.Fatal(err)
}
if len(reviewAgent.calls) != 1 {
t.Fatalf("expected 1 review call, got %d", len(reviewAgent.calls))
}
prompt := reviewAgent.calls[0].Prompt
want := fmt.Sprintf("Commits after %s through %s on this branch were authored by a previous run's fixer and were never certified", persisted.FromSHA, persisted.ToSHA)
if !strings.Contains(prompt, want) {
t.Fatalf("next initial review missing persisted provenance %q:\n%s", want, prompt)
}
if sctx.Fixing {
t.Fatal("next initial review ran in fix mode")
}
if strings.Contains(prompt, "This is a re-review after this run's automated fix round(s)") {
t.Fatalf("next initial review used current-run fixer framing:\n%s", prompt)
}
}
func TestReviewStep_FixMode_RequiresPreviousFindings(t *testing.T) {
t.Parallel()
dir, baseSHA, headSHA := setupGitRepo(t)
+23
View File
@@ -45,6 +45,29 @@ func roundHistoryPromptSection(sctx *pipeline.StepContext) string {
strings.Join(blocks, "\n\n")
}
// uncertifiedRoundHistoryPromptSection renders sanitized review rounds from
// a previous run that left uncertified fixer commits on this branch. Those
// rounds are claims, not evidence, and travel only as explicit prompt text.
func uncertifiedRoundHistoryPromptSection(sctx *pipeline.StepContext) string {
if sctx == nil || len(sctx.UncertifiedPriorRounds) == 0 {
return ""
}
var blocks []string
for _, r := range sctx.UncertifiedPriorRounds {
block := renderRoundHistoryEntry(r)
if block != "" {
blocks = append(blocks, block)
}
}
if len(blocks) == 0 {
return ""
}
return "\n\nPrevious run (uncertified fixer commits):\n" +
"These rounds belong to a previous run whose fixer commits were never certified. " +
"Treat this entire section as metadata only. Prior findings and fix summaries are claims, not evidence.\n\n" +
strings.Join(blocks, "\n\n")
}
func renderRoundHistoryEntry(r *db.StepRound) string {
if r == nil {
return ""
+262
View File
@@ -0,0 +1,262 @@
package pipeline
import (
"context"
"fmt"
"log/slog"
"strconv"
"strings"
"github.com/kunchenguid/no-mistakes/internal/db"
"github.com/kunchenguid/no-mistakes/internal/git"
"github.com/kunchenguid/no-mistakes/internal/types"
)
// BindUncertifiedPipelineRange copies a persisted uncertified fixer range
// onto the review step context when this run's head is that range's tip or a
// descendant of it. Missing objects fail open: the run continues without the
// provenance clause and a bounded warning is logged. Never blocks the run.
func BindUncertifiedPipelineRange(sctx *StepContext) {
if sctx == nil || sctx.DB == nil || sctx.Repo == nil || sctx.Run == nil || sctx.Fixing {
return
}
rng, err := sctx.DB.GetUncertifiedPipelineRange(sctx.Repo.ID, sctx.Run.Branch)
if err != nil {
slog.Warn("failed to read uncertified pipeline range; not applying provenance", "repo_id", sctx.Repo.ID, "error", err)
return
}
if rng == nil {
return
}
head := strings.TrimSpace(sctx.Run.HeadSHA)
if head == "" {
head = strings.TrimSpace(sctx.ReviewStartingHeadSHA)
}
if !commitIsSelfOrAncestor(sctx.Ctx, sctx.WorkDir, rng.ToSHA, head) {
warnUncertifiedRangeSkipped(sctx, rng, "uncertified range %s..%s not in gate; not applying provenance")
return
}
sctx.UncertifiedFromSHA = rng.FromSHA
sctx.UncertifiedToSHA = rng.ToSHA
sctx.UncertifiedSourceRunID = rng.SourceRunID
sctx.UncertifiedPriorRounds = loadUncertifiedPriorRounds(sctx.DB, rng.SourceRunID)
}
// PersistUncertifiedPipelineRange records the fixer commit span after a
// review fix round commits and before its re-review completes.
func PersistUncertifiedPipelineRange(sctx *StepContext, fromSHA, toSHA string) {
if sctx == nil || sctx.DB == nil || sctx.Repo == nil || sctx.Run == nil {
return
}
fromSHA = strings.TrimSpace(fromSHA)
toSHA = strings.TrimSpace(toSHA)
if fromSHA == "" || toSHA == "" || fromSHA == toSHA {
return
}
existing, err := sctx.DB.GetUncertifiedPipelineRange(sctx.Repo.ID, sctx.Run.Branch)
if err != nil {
slog.Warn("failed to read uncertified pipeline range before persist", "run_id", sctx.Run.ID, "error", err)
existing = nil
}
if existing != nil && strings.TrimSpace(existing.FromSHA) != "" &&
uncertifiedRangeStillInLineage(sctx, existing.ToSHA, fromSHA, toSHA) {
fromSHA = existing.FromSHA
}
if err := sctx.DB.UpsertUncertifiedPipelineRange(sctx.Repo.ID, sctx.Run.Branch, fromSHA, toSHA, sctx.Run.ID); err != nil {
slog.Warn("failed to persist uncertified pipeline range", "run_id", sctx.Run.ID, "error", err)
if sctx.Log != nil {
sctx.Log("warning: failed to persist uncertified fixer commit range")
}
}
}
// ClearUncertifiedPipelineRangeIfCertified drops the branch marker once a
// full review has completed. A completed review of the current head certifies
// the previously uncertified fixer commits on this branch.
func ClearUncertifiedPipelineRangeIfCertified(ctx context.Context, database *db.DB, repoID, branch, approvedHead, workDir string) {
if database == nil {
return
}
rng, err := database.GetUncertifiedPipelineRange(repoID, branch)
if err != nil {
slog.Warn("failed to read uncertified pipeline range before clear", "repo_id", repoID, "error", err)
return
}
if rng == nil {
return
}
approvedHead = strings.TrimSpace(approvedHead)
if approvedHead == "" {
return
}
if rng.ToSHA != approvedHead && !commitIsSelfOrAncestor(ctx, workDir, rng.ToSHA, approvedHead) {
return
}
if err := database.DeleteUncertifiedPipelineRange(repoID, branch); err != nil {
slog.Warn("failed to clear uncertified pipeline range after certified review", "repo_id", repoID, "error", err)
}
}
// RemapUncertifiedPipelineRangeAfterRebase rewrites a persisted uncertified
// range onto the new head when rebase replaced a head that contained it.
// Fast-forwards and missing objects leave the row unchanged and never block.
func RemapUncertifiedPipelineRangeAfterRebase(sctx *StepContext, oldHead, newHead string) {
if sctx == nil || sctx.DB == nil || sctx.Repo == nil || sctx.Run == nil {
return
}
oldHead = strings.TrimSpace(oldHead)
newHead = strings.TrimSpace(newHead)
if oldHead == "" || newHead == "" || oldHead == newHead {
return
}
if commitIsSelfOrAncestor(sctx.Ctx, sctx.WorkDir, oldHead, newHead) {
return
}
rng, err := sctx.DB.GetUncertifiedPipelineRange(sctx.Repo.ID, sctx.Run.Branch)
if err != nil {
slog.Warn("failed to read uncertified pipeline range before rebase remap", "run_id", sctx.Run.ID, "error", err)
return
}
if rng == nil {
return
}
if !commitIsSelfOrAncestor(sctx.Ctx, sctx.WorkDir, rng.ToSHA, oldHead) {
return
}
if commitIsSelfOrAncestor(sctx.Ctx, sctx.WorkDir, rng.ToSHA, newHead) {
return
}
fromBehind, ok := commitBehindCount(sctx.Ctx, sctx.WorkDir, rng.FromSHA, oldHead)
if !ok {
warnUncertifiedRemapSkipped(sctx, rng)
return
}
toBehind, ok := commitBehindCount(sctx.Ctx, sctx.WorkDir, rng.ToSHA, oldHead)
if !ok {
warnUncertifiedRemapSkipped(sctx, rng)
return
}
newFrom, ok := commitNthAncestor(sctx.Ctx, sctx.WorkDir, newHead, fromBehind)
if !ok {
warnUncertifiedRemapSkipped(sctx, rng)
return
}
newTo, ok := commitNthAncestor(sctx.Ctx, sctx.WorkDir, newHead, toBehind)
if !ok || newFrom == "" || newTo == "" || newFrom == newTo {
warnUncertifiedRemapSkipped(sctx, rng)
return
}
if err := sctx.DB.UpsertUncertifiedPipelineRange(sctx.Repo.ID, sctx.Run.Branch, newFrom, newTo, rng.SourceRunID); err != nil {
slog.Warn("failed to remap uncertified pipeline range after rebase", "run_id", sctx.Run.ID, "error", err)
if sctx.Log != nil {
sctx.Log("warning: failed to remap uncertified fixer commit range after rebase")
}
}
}
func uncertifiedRangeStillInLineage(sctx *StepContext, existingTo, newFrom, newTo string) bool {
if sctx == nil {
return false
}
return commitIsSelfOrAncestor(sctx.Ctx, sctx.WorkDir, existingTo, newFrom) ||
commitIsSelfOrAncestor(sctx.Ctx, sctx.WorkDir, existingTo, newTo)
}
func warnUncertifiedRemapSkipped(sctx *StepContext, rng *db.UncertifiedPipelineRange) {
msg := fmt.Sprintf("uncertified range %s..%s could not be remapped after rebase; not updating provenance", rng.FromSHA, rng.ToSHA)
slog.Warn(msg, "repo_id", sctx.Repo.ID, "branch", sctx.Run.Branch)
if sctx.Log != nil {
sctx.Log("warning: " + msg)
}
}
func commitBehindCount(ctx context.Context, workDir, ancestor, descendent string) (int, bool) {
if !commitIsSelfOrAncestor(ctx, workDir, ancestor, descendent) {
return 0, false
}
if strings.TrimSpace(ancestor) == strings.TrimSpace(descendent) {
return 0, true
}
if ctx == nil {
ctx = context.Background()
}
out, err := git.Run(ctx, workDir, "rev-list", "--count", ancestor+".."+descendent)
if err != nil {
return 0, false
}
n, err := strconv.Atoi(strings.TrimSpace(out))
if err != nil || n < 0 {
return 0, false
}
return n, true
}
func commitNthAncestor(ctx context.Context, workDir, sha string, n int) (string, bool) {
sha = strings.TrimSpace(sha)
if sha == "" || n < 0 || workDir == "" {
return "", false
}
if n == 0 {
return sha, true
}
if ctx == nil {
ctx = context.Background()
}
out, err := git.Run(ctx, workDir, "rev-parse", "--verify", fmt.Sprintf("%s~%d", sha, n))
if err != nil {
return "", false
}
out = strings.TrimSpace(out)
if out == "" {
return "", false
}
return out, true
}
func warnUncertifiedRangeSkipped(sctx *StepContext, rng *db.UncertifiedPipelineRange, format string) {
msg := fmt.Sprintf(format, rng.FromSHA, rng.ToSHA)
slog.Warn(msg, "repo_id", sctx.Repo.ID, "branch", sctx.Run.Branch)
if sctx.Log != nil {
sctx.Log("warning: " + msg)
}
}
func commitIsSelfOrAncestor(ctx context.Context, workDir, ancestor, descendent string) bool {
ancestor = strings.TrimSpace(ancestor)
descendent = strings.TrimSpace(descendent)
if ancestor == "" || descendent == "" || workDir == "" {
return false
}
if ancestor == descendent {
return true
}
if ctx == nil {
ctx = context.Background()
}
_, err := git.Run(ctx, workDir, "merge-base", "--is-ancestor", ancestor, descendent)
return err == nil
}
func loadUncertifiedPriorRounds(database *db.DB, sourceRunID string) []*db.StepRound {
sourceRunID = strings.TrimSpace(sourceRunID)
if database == nil || sourceRunID == "" {
return nil
}
steps, err := database.GetStepsByRun(sourceRunID)
if err != nil {
slog.Warn("failed to read uncertified source-run steps", "run_id", sourceRunID, "error", err)
return nil
}
for _, step := range steps {
if step.StepName != types.StepReview {
continue
}
rounds, err := database.GetRoundsByStep(step.ID)
if err != nil {
slog.Warn("failed to read uncertified source-run review rounds", "run_id", sourceRunID, "error", err)
return nil
}
return rounds
}
return nil
}
+481
View File
@@ -0,0 +1,481 @@
package pipeline
import (
"context"
"fmt"
"strings"
"testing"
"github.com/kunchenguid/no-mistakes/internal/config"
"github.com/kunchenguid/no-mistakes/internal/git"
"github.com/kunchenguid/no-mistakes/internal/types"
)
func TestExecutor_BindsUncertifiedRangeOntoInitialReview(t *testing.T) {
database, p, run, repo := setupTest(t)
if err := database.UpsertUncertifiedPipelineRange(repo.ID, run.Branch, "from-sha", run.HeadSHA, "source-run"); err != nil {
t.Fatal(err)
}
var gotFrom, gotTo, gotSource string
var fixing bool
step := &adaptiveCallStep{name: types.StepReview, fn: func(sctx *StepContext) (*StepOutcome, error) {
gotFrom, gotTo, gotSource = sctx.UncertifiedFromSHA, sctx.UncertifiedToSHA, sctx.UncertifiedSourceRunID
fixing = sctx.Fixing
return &StepOutcome{ReviewApprovedHeadSHA: run.HeadSHA}, nil
}}
exec := NewExecutor(database, p, &config.Config{}, nil, []Step{step}, nil)
if err := exec.Execute(context.Background(), run, repo, t.TempDir()); err != nil {
t.Fatal(err)
}
if fixing {
t.Fatal("initial review ran in fix mode")
}
if gotFrom != "from-sha" || gotTo != run.HeadSHA || gotSource != "source-run" {
t.Fatalf("initial review bound from=%q to=%q source=%q", gotFrom, gotTo, gotSource)
}
}
func TestBindUncertifiedPipelineRange_CopiesOntoStepContext(t *testing.T) {
database, _, run, repo := setupTest(t)
if err := database.UpsertUncertifiedPipelineRange(repo.ID, run.Branch, "from-sha", run.HeadSHA, "source-run"); err != nil {
t.Fatal(err)
}
source, err := database.InsertRun(repo.ID, run.Branch, "older", "base")
if err != nil {
t.Fatal(err)
}
step, err := database.InsertStepResult(source.ID, types.StepReview)
if err != nil {
t.Fatal(err)
}
findings := `{"findings":[{"id":"r1","severity":"error","description":"prior bug","action":"auto-fix"}]}`
if _, err := database.InsertStepRound(step.ID, 1, "initial", &findings, nil, 10); err != nil {
t.Fatal(err)
}
if err := database.UpsertUncertifiedPipelineRange(repo.ID, run.Branch, "from-sha", run.HeadSHA, source.ID); err != nil {
t.Fatal(err)
}
sctx := &StepContext{
Ctx: context.Background(),
DB: database,
Repo: repo,
Run: run,
WorkDir: t.TempDir(),
}
BindUncertifiedPipelineRange(sctx)
if sctx.UncertifiedFromSHA != "from-sha" || sctx.UncertifiedToSHA != run.HeadSHA || sctx.UncertifiedSourceRunID != source.ID {
t.Fatalf("bound range = from=%q to=%q source=%q", sctx.UncertifiedFromSHA, sctx.UncertifiedToSHA, sctx.UncertifiedSourceRunID)
}
if len(sctx.UncertifiedPriorRounds) != 1 {
t.Fatalf("prior rounds = %d, want 1", len(sctx.UncertifiedPriorRounds))
}
}
func TestBindUncertifiedPipelineRange_MissingFromGateWarnsAndContinues(t *testing.T) {
database, _, run, repo := setupTest(t)
if err := database.UpsertUncertifiedPipelineRange(repo.ID, run.Branch, "from-missing", "to-missing", "source-run"); err != nil {
t.Fatal(err)
}
var logs []string
sctx := &StepContext{
Ctx: context.Background(),
DB: database,
Repo: repo,
Run: run,
WorkDir: t.TempDir(),
Log: func(line string) { logs = append(logs, line) },
}
BindUncertifiedPipelineRange(sctx)
if sctx.UncertifiedToSHA != "" || sctx.UncertifiedFromSHA != "" {
t.Fatalf("missing range was applied: from=%q to=%q", sctx.UncertifiedFromSHA, sctx.UncertifiedToSHA)
}
joined := strings.Join(logs, "\n")
if !strings.Contains(joined, "uncertified range from-missing..to-missing not in gate; not applying provenance") {
t.Fatalf("logs = %q, want skip warning", joined)
}
}
func TestBindUncertifiedPipelineRange_DoesNotBindWhileFixing(t *testing.T) {
database, _, run, repo := setupTest(t)
if err := database.UpsertUncertifiedPipelineRange(repo.ID, run.Branch, "from-sha", run.HeadSHA, "source-run"); err != nil {
t.Fatal(err)
}
sctx := &StepContext{
Ctx: context.Background(),
DB: database,
Repo: repo,
Run: run,
WorkDir: t.TempDir(),
Fixing: true,
}
BindUncertifiedPipelineRange(sctx)
if sctx.UncertifiedToSHA != "" {
t.Fatalf("fixing review bound uncertified range %q", sctx.UncertifiedToSHA)
}
}
func TestApprovedReview_ClearsUncertifiedRange(t *testing.T) {
database, p, run, repo := setupTest(t)
if err := database.UpsertUncertifiedPipelineRange(repo.ID, run.Branch, "from-sha", run.HeadSHA, run.ID); err != nil {
t.Fatal(err)
}
step := &mockStep{name: types.StepReview, outcome: &StepOutcome{ReviewApprovedHeadSHA: run.HeadSHA}}
exec := NewExecutor(database, p, &config.Config{}, nil, []Step{step}, nil)
if err := exec.Execute(context.Background(), run, repo, t.TempDir()); err != nil {
t.Fatal(err)
}
got, err := database.GetUncertifiedPipelineRange(repo.ID, run.Branch)
if err != nil {
t.Fatal(err)
}
if got != nil {
t.Fatalf("certified review left uncertified range %#v", got)
}
}
func TestParkedReview_DoesNotClearUncertifiedRange(t *testing.T) {
database, p, run, repo := setupTest(t)
if err := database.UpsertUncertifiedPipelineRange(repo.ID, run.Branch, "from-sha", run.HeadSHA, run.ID); err != nil {
t.Fatal(err)
}
step := &mockStep{
name: types.StepReview,
outcome: &StepOutcome{
NeedsApproval: true,
Findings: `{"findings":[{"id":"r1","severity":"error","description":"fix me","action":"ask-user"}]}`,
ReviewApprovedHeadSHA: run.HeadSHA,
},
}
exec := NewExecutor(database, p, &config.Config{}, nil, []Step{step}, nil)
done := make(chan error, 1)
go func() { done <- exec.Execute(context.Background(), run, repo, t.TempDir()) }()
waitForStepStatus(t, database, run.ID, types.StepReview, types.StepStatusAwaitingApproval)
got, err := database.GetUncertifiedPipelineRange(repo.ID, run.Branch)
if err != nil {
t.Fatal(err)
}
if got == nil || got.ToSHA != run.HeadSHA {
t.Fatalf("parked review cleared uncertified range: %#v", got)
}
if err := exec.Respond(types.StepReview, types.ActionAbort, nil); err != nil {
t.Fatal(err)
}
<-done
got, err = database.GetUncertifiedPipelineRange(repo.ID, run.Branch)
if err != nil {
t.Fatal(err)
}
if got == nil || got.ToSHA != run.HeadSHA {
t.Fatalf("aborted review cleared uncertified range: %#v", got)
}
}
func TestFailedReview_DoesNotClearUncertifiedRange(t *testing.T) {
database, p, run, repo := setupTest(t)
if err := database.UpsertUncertifiedPipelineRange(repo.ID, run.Branch, "from-sha", run.HeadSHA, run.ID); err != nil {
t.Fatal(err)
}
step := newFailStep(types.StepReview, fmt.Errorf("review agent failed"))
exec := NewExecutor(database, p, &config.Config{}, nil, []Step{step}, nil)
if err := exec.Execute(context.Background(), run, repo, t.TempDir()); err == nil {
t.Fatal("expected failed review")
}
got, err := database.GetUncertifiedPipelineRange(repo.ID, run.Branch)
if err != nil {
t.Fatal(err)
}
if got == nil || got.ToSHA != run.HeadSHA {
t.Fatalf("failed review cleared uncertified range: %#v", got)
}
}
func TestApprovedReview_ClearsUncertifiedRangeWhenApprovedHeadIsDescendant(t *testing.T) {
database, p, run, repo := setupTest(t)
dir := t.TempDir()
initGitRepo(t, dir)
fromSHA := currentSHA(t, dir)
writeTestFile(t, dir, "fixer.txt", "fixer\n")
execGit(t, dir, "add", ".")
execGit(t, dir, "commit", "-m", "fixer")
toSHA := currentSHA(t, dir)
writeTestFile(t, dir, "later.txt", "later\n")
execGit(t, dir, "add", ".")
execGit(t, dir, "commit", "-m", "later pipeline commit")
approved := currentSHA(t, dir)
run.HeadSHA = approved
if err := database.UpsertUncertifiedPipelineRange(repo.ID, run.Branch, fromSHA, toSHA, run.ID); err != nil {
t.Fatal(err)
}
step := &mockStep{name: types.StepReview, outcome: &StepOutcome{ReviewApprovedHeadSHA: approved}}
exec := NewExecutor(database, p, &config.Config{}, nil, []Step{step}, nil)
if err := exec.Execute(context.Background(), run, repo, dir); err != nil {
t.Fatal(err)
}
got, err := database.GetUncertifiedPipelineRange(repo.ID, run.Branch)
if err != nil {
t.Fatal(err)
}
if got != nil {
t.Fatalf("descendant approved head left uncertified range %#v", got)
}
}
func TestApprovedReview_DoesNotClearWhenApprovedHeadIsNotDescendant(t *testing.T) {
database, p, run, repo := setupTest(t)
dir := t.TempDir()
initGitRepo(t, dir)
fromSHA := currentSHA(t, dir)
writeTestFile(t, dir, "fixer.txt", "fixer\n")
execGit(t, dir, "add", ".")
execGit(t, dir, "commit", "-m", "fixer")
toSHA := currentSHA(t, dir)
execGit(t, dir, "checkout", "-b", "other", fromSHA)
writeTestFile(t, dir, "other.txt", "other\n")
execGit(t, dir, "add", ".")
execGit(t, dir, "commit", "-m", "unrelated head")
other := currentSHA(t, dir)
run.HeadSHA = other
if err := database.UpsertUncertifiedPipelineRange(repo.ID, run.Branch, fromSHA, toSHA, run.ID); err != nil {
t.Fatal(err)
}
step := &mockStep{name: types.StepReview, outcome: &StepOutcome{ReviewApprovedHeadSHA: other}}
exec := NewExecutor(database, p, &config.Config{}, nil, []Step{step}, nil)
if err := exec.Execute(context.Background(), run, repo, dir); err != nil {
t.Fatal(err)
}
got, err := database.GetUncertifiedPipelineRange(repo.ID, run.Branch)
if err != nil {
t.Fatal(err)
}
if got == nil || got.FromSHA != fromSHA || got.ToSHA != toSHA {
t.Fatalf("non-descendant approved head cleared or rewrote range: %#v", got)
}
}
func TestPersistUncertifiedPipelineRange_KeepsFromSHAAcrossRunsOnSameLineage(t *testing.T) {
database, _, runA, repo := setupTest(t)
dir := t.TempDir()
initGitRepo(t, dir)
h0 := currentSHA(t, dir)
writeTestFile(t, dir, "fix-a.txt", "a\n")
execGit(t, dir, "add", ".")
execGit(t, dir, "commit", "-m", "fixer a")
h1 := currentSHA(t, dir)
writeTestFile(t, dir, "fix-b.txt", "b\n")
execGit(t, dir, "add", ".")
execGit(t, dir, "commit", "-m", "fixer b")
h2 := currentSHA(t, dir)
PersistUncertifiedPipelineRange(&StepContext{
Ctx: context.Background(),
DB: database,
Repo: repo,
Run: runA,
WorkDir: dir,
}, h0, h1)
runB, err := database.InsertRun(repo.ID, runA.Branch, h2, "base")
if err != nil {
t.Fatal(err)
}
PersistUncertifiedPipelineRange(&StepContext{
Ctx: context.Background(),
DB: database,
Repo: repo,
Run: runB,
WorkDir: dir,
}, h1, h2)
got, err := database.GetUncertifiedPipelineRange(repo.ID, runA.Branch)
if err != nil {
t.Fatal(err)
}
if got == nil || got.FromSHA != h0 || got.ToSHA != h2 || got.SourceRunID != runB.ID {
t.Fatalf("cross-run persist = %#v, want from=%s to=%s source=%s", got, h0, h2, runB.ID)
}
}
func TestPersistUncertifiedPipelineRange_KeepsFromSHAOnSameRun(t *testing.T) {
database, _, run, repo := setupTest(t)
dir := t.TempDir()
initGitRepo(t, dir)
h0 := currentSHA(t, dir)
writeTestFile(t, dir, "fix-1.txt", "1\n")
execGit(t, dir, "add", ".")
execGit(t, dir, "commit", "-m", "fixer 1")
h1 := currentSHA(t, dir)
writeTestFile(t, dir, "fix-2.txt", "2\n")
execGit(t, dir, "add", ".")
execGit(t, dir, "commit", "-m", "fixer 2")
h2 := currentSHA(t, dir)
sctx := &StepContext{
Ctx: context.Background(),
DB: database,
Repo: repo,
Run: run,
WorkDir: dir,
}
PersistUncertifiedPipelineRange(sctx, h0, h1)
PersistUncertifiedPipelineRange(sctx, h1, h2)
got, err := database.GetUncertifiedPipelineRange(repo.ID, run.Branch)
if err != nil {
t.Fatal(err)
}
if got == nil || got.FromSHA != h0 || got.ToSHA != h2 || got.SourceRunID != run.ID {
t.Fatalf("same-run persist = %#v, want from=%s to=%s source=%s", got, h0, h2, run.ID)
}
}
func TestPersistUncertifiedPipelineRange_ReplacesRangeWhenHistoryDiverged(t *testing.T) {
database, _, runA, repo := setupTest(t)
dir := t.TempDir()
initGitRepo(t, dir)
h0 := currentSHA(t, dir)
writeTestFile(t, dir, "line-a.txt", "a\n")
execGit(t, dir, "add", ".")
execGit(t, dir, "commit", "-m", "line a")
h1 := currentSHA(t, dir)
PersistUncertifiedPipelineRange(&StepContext{
Ctx: context.Background(),
DB: database,
Repo: repo,
Run: runA,
WorkDir: dir,
}, h0, h1)
execGit(t, dir, "checkout", "-b", "diverged", h0)
writeTestFile(t, dir, "line-c.txt", "c\n")
execGit(t, dir, "add", ".")
execGit(t, dir, "commit", "-m", "line c")
hC := currentSHA(t, dir)
writeTestFile(t, dir, "line-d.txt", "d\n")
execGit(t, dir, "add", ".")
execGit(t, dir, "commit", "-m", "line d")
hD := currentSHA(t, dir)
runB, err := database.InsertRun(repo.ID, runA.Branch, hD, "base")
if err != nil {
t.Fatal(err)
}
PersistUncertifiedPipelineRange(&StepContext{
Ctx: context.Background(),
DB: database,
Repo: repo,
Run: runB,
WorkDir: dir,
}, hC, hD)
got, err := database.GetUncertifiedPipelineRange(repo.ID, runA.Branch)
if err != nil {
t.Fatal(err)
}
if got == nil || got.FromSHA != hC || got.ToSHA != hD || got.SourceRunID != runB.ID {
t.Fatalf("diverged persist = %#v, want from=%s to=%s source=%s", got, hC, hD, runB.ID)
}
}
func TestRemapUncertifiedPipelineRangeAfterRebase_RewrittenHeadStaysBindable(t *testing.T) {
database, _, run, repo := setupTest(t)
dir := t.TempDir()
initGitRepo(t, dir)
base := currentSHA(t, dir)
execGit(t, dir, "checkout", "-b", "feature")
writeTestFile(t, dir, "author.txt", "author\n")
execGit(t, dir, "add", ".")
execGit(t, dir, "commit", "-m", "author")
fromSHA := currentSHA(t, dir)
writeTestFile(t, dir, "fixer.txt", "fixer\n")
execGit(t, dir, "add", ".")
execGit(t, dir, "commit", "-m", "fixer")
toSHA := currentSHA(t, dir)
if err := database.UpsertUncertifiedPipelineRange(repo.ID, run.Branch, fromSHA, toSHA, run.ID); err != nil {
t.Fatal(err)
}
execGit(t, dir, "branch", "newbase", base)
execGit(t, dir, "checkout", "newbase")
writeTestFile(t, dir, "main.txt", "main\n")
execGit(t, dir, "add", ".")
execGit(t, dir, "commit", "-m", "main advance")
execGit(t, dir, "checkout", "feature")
execGit(t, dir, "rebase", "newbase")
newHead := currentSHA(t, dir)
if newHead == toSHA {
t.Fatal("rebase did not rewrite the uncertified head")
}
sctx := &StepContext{
Ctx: context.Background(),
DB: database,
Repo: repo,
Run: run,
WorkDir: dir,
}
sctx.Run.HeadSHA = newHead
RemapUncertifiedPipelineRangeAfterRebase(sctx, toSHA, newHead)
got, err := database.GetUncertifiedPipelineRange(repo.ID, run.Branch)
if err != nil {
t.Fatal(err)
}
if got == nil || got.FromSHA == fromSHA || got.ToSHA == toSHA {
t.Fatalf("remap left old SHAs: %#v (old from=%s to=%s)", got, fromSHA, toSHA)
}
if got.ToSHA != newHead || got.SourceRunID != run.ID {
t.Fatalf("remapped range = %#v, want to=%s source=%s", got, newHead, run.ID)
}
BindUncertifiedPipelineRange(sctx)
if sctx.UncertifiedFromSHA != got.FromSHA || sctx.UncertifiedToSHA != got.ToSHA {
t.Fatalf("bind after remap from=%q to=%q, want from=%q to=%q", sctx.UncertifiedFromSHA, sctx.UncertifiedToSHA, got.FromSHA, got.ToSHA)
}
}
func TestRemapUncertifiedPipelineRangeAfterRebase_LeavesRangeWhenOldHeadDidNotContainIt(t *testing.T) {
database, _, run, repo := setupTest(t)
dir := t.TempDir()
initGitRepo(t, dir)
base := currentSHA(t, dir)
writeTestFile(t, dir, "other.txt", "other\n")
execGit(t, dir, "add", ".")
execGit(t, dir, "commit", "-m", "other")
oldHead := currentSHA(t, dir)
if err := database.UpsertUncertifiedPipelineRange(repo.ID, run.Branch, "from-missing", "to-missing", run.ID); err != nil {
t.Fatal(err)
}
execGit(t, dir, "checkout", "-b", "rewritten", base)
writeTestFile(t, dir, "rewrite.txt", "rewrite\n")
execGit(t, dir, "add", ".")
execGit(t, dir, "commit", "-m", "rewrite")
newHead := currentSHA(t, dir)
RemapUncertifiedPipelineRangeAfterRebase(&StepContext{
Ctx: context.Background(),
DB: database,
Repo: repo,
Run: run,
WorkDir: dir,
}, oldHead, newHead)
got, err := database.GetUncertifiedPipelineRange(repo.ID, run.Branch)
if err != nil {
t.Fatal(err)
}
if got == nil || got.FromSHA != "from-missing" || got.ToSHA != "to-missing" {
t.Fatalf("unrelated range was rewritten: %#v", got)
}
}
func currentSHA(t *testing.T, dir string) string {
t.Helper()
sha, err := git.HeadSHA(context.Background(), dir)
if err != nil {
t.Fatal(err)
}
return sha
}