diff --git a/README.md b/README.md index 051fdea..5ebe724 100644 --- a/README.md +++ b/README.md @@ -96,6 +96,9 @@ Secrets are never written to `config.yml`. Non-secret config lives in the `codereview` config directory resolved by the operating system, and durable review data lives under the OS data directory for the `cr` binary. +For repo review guidance, reviewer-facing dossier context, and dossier/workbench +retention conventions, see [docs/review-guidance.md](docs/review-guidance.md). + ## Authentication And Setup `cr` v1 supports GitHub personal access token authentication for Git-host diff --git a/docs/review-guidance.md b/docs/review-guidance.md new file mode 100644 index 0000000..3fae9e8 --- /dev/null +++ b/docs/review-guidance.md @@ -0,0 +1,75 @@ +# Review Guidance And Run Artifacts + +This document describes the user-facing conventions for repo review guidance, +generated review context, and cleanup behavior in `cr review`. + +## Repo Review Guidance + +Today, repo-owned review guidance lives in trusted repo-local agent definitions +under `.codereview/agents/`. + +Those agent definitions can shape review behavior by: + +- adding repository-specific reviewers +- narrowing which file patterns a reviewer applies to +- carrying repository-specific prompt guidance for those reviewers + +`cr review` does not load repo guidance from the PR head for the same review +run. It reads `.codereview/agents/` from the PR base branch, pins that source +to the resolved base SHA, and uses that pinned base-branch guidance throughout +the run. + +That means a PR which changes `.codereview/agents/` does not change its own +review behavior unless the same guidance is already present on the base branch. + +## Guidance Provenance In The Dossier + +Checkout-native review writes reviewer-facing dossier artifacts under the run +artifact directory. The file `dossier/final/repo-guidance.md` records: + +- that repo review guidance comes from `.codereview/agents/` +- the provenance label for the pinned base-branch source +- the trust-boundary note that PR-head guidance changes do not affect that run +- whether the base branch guidance source was available or missing + +This file is intended for reviewers and operators who need to understand which +repo guidance influenced a review without reading pipeline code. + +## Reviewer-Facing Context + +Reviewer-facing dossier files are the parts of generated context that help an +agent review the code change itself. This typically includes: + +- PR intent +- changed file paths and basic change-map details +- repo guidance provenance +- relevant top-level comments +- inline discussion summaries anchored to files and lines + +Reviewer-facing dossier files intentionally exclude harness bookkeeping such as: + +- session IDs and provider session handles +- retry and resume bookkeeping +- posting mode and outbox state +- retention metadata +- other internal runtime fields that do not improve review judgment + +## Run Artifacts And Cleanup + +Checkout-native review stores dossier and workbench artifacts under the normal +review run artifact root: + +```text +runs// + dossier/ + workbench/ +``` + +Because those directories live under the run artifact root, existing data +lifecycle commands clean them up automatically: + +- `cr data prune` removes dossier and workbench directories for pruned runs +- `cr data purge` removes the entire data root, including dossier and workbench + artifacts + +No separate retention setting is required for dossier or workbench cleanup. diff --git a/internal/cmd/datacmd/datacmd_test.go b/internal/cmd/datacmd/datacmd_test.go index d171300..785c742 100644 --- a/internal/cmd/datacmd/datacmd_test.go +++ b/internal/cmd/datacmd/datacmd_test.go @@ -300,6 +300,40 @@ func TestDataPruneKeepLastPerPostMode(t *testing.T) { } } +func TestDataPruneRemovesDossierAndWorkbenchArtifacts(t *testing.T) { + statedirtest.Hermetic(t) + layout := mustLayout(t) + store := openLedgerForTest(t, layout) + run := allocateRun(t, store, layout, "live-old", ledger.PostModeLive, testNow().Add(-91*24*time.Hour)) + if err := store.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + writeDataCommandFile(t, filepath.Join(run.ArtifactPath, "dossier", "final", "repo-guidance.md"), "guidance") + writeDataCommandFile(t, filepath.Join(run.ArtifactPath, "workbench", "repo", "main.go"), "package main\n") + writeDataCommandFile(t, filepath.Join(run.ArtifactPath, "workbench", "scratch", "notes.txt"), "scratch") + + var stdout, stderr bytes.Buffer + if err := runDataCommand(&stdout, &stderr, "data", "prune", "--older-than", "1h", "--json"); err != nil { + t.Fatalf("data prune: %v; stderr = %q", err, stderr.String()) + } + var pruned view.DataPrune + if err := json.Unmarshal(stdout.Bytes(), &pruned); err != nil { + t.Fatalf("Unmarshal prune: %v; stdout = %q", err, stdout.String()) + } + if len(pruned.DeletedRuns) != 1 || pruned.DeletedRuns[0].RunID != "live-old" { + t.Fatalf("deleted runs = %#v, want live-old deletion", pruned.DeletedRuns) + } + if _, err := os.Stat(filepath.Join(run.ArtifactPath, "dossier")); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("dossier stat err = %v, want removed", err) + } + if _, err := os.Stat(filepath.Join(run.ArtifactPath, "workbench")); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("workbench stat err = %v, want removed", err) + } + if _, err := os.Stat(run.ArtifactPath); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("artifact root stat err = %v, want removed", err) + } +} + func TestDataPruneRejectsInvalidFlagCombinations(t *testing.T) { statedirtest.Hermetic(t) tests := []struct { @@ -371,6 +405,36 @@ func TestDataPurgeDryRunDoesNotRequireConfirmationOrDelete(t *testing.T) { } } +func TestDataPurgeRemovesDossierAndWorkbenchArtifacts(t *testing.T) { + statedirtest.Hermetic(t) + layout := mustLayout(t) + store := openLedgerForTest(t, layout) + run := allocateRun(t, store, layout, "live-old", ledger.PostModeLive, testNow().Add(-91*24*time.Hour)) + if err := store.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + writeDataCommandFile(t, filepath.Join(run.ArtifactPath, "dossier", "final", "repo-guidance.md"), "guidance") + writeDataCommandFile(t, filepath.Join(run.ArtifactPath, "workbench", "repo", "main.go"), "package main\n") + writeDataCommandFile(t, filepath.Join(run.ArtifactPath, "workbench", "scratch", "notes.txt"), "scratch") + + var stdout, stderr bytes.Buffer + if err := runDataCommand(&stdout, &stderr, "data", "purge", "--yes"); err != nil { + t.Fatalf("data purge: %v; stderr = %q", err, stderr.String()) + } + if !strings.Contains(stdout.String(), "Purged data root:") { + t.Fatalf("stdout = %q, want purge summary", stdout.String()) + } + if _, err := os.Stat(filepath.Join(run.ArtifactPath, "dossier")); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("dossier stat err = %v, want removed", err) + } + if _, err := os.Stat(filepath.Join(run.ArtifactPath, "workbench")); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("workbench stat err = %v, want removed", err) + } + if _, err := os.Stat(layout.DataRoot); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("data root stat err = %v, want removed", err) + } +} + func TestDataPurgeRequiresConfirmation(t *testing.T) { statedirtest.Hermetic(t) var stdout, stderr bytes.Buffer diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go index 5cd7f51..8ebc7dc 100644 --- a/internal/pipeline/pipeline.go +++ b/internal/pipeline/pipeline.go @@ -4499,19 +4499,56 @@ func renderDossierChangeMap(files []dossierChangedFileArtifact) string { func renderDossierRepoGuidance(repo dossierRepoContextArtifact) string { var out strings.Builder out.WriteString("# Repo Guidance\n\n") - out.WriteString("No dedicated repo review-guidance source is defined yet.\n\n") - if repo.RepoInfo != nil { - out.WriteString("Repo-local agent provenance: ") - out.WriteString(repo.RepoInfo.Provenance) - out.WriteString("\n\n") - if note := strings.TrimSpace(repo.RepoInfo.TrustNote()); note != "" { - out.WriteString(note) - out.WriteString("\n") + out.WriteString("Repo review guidance for this run comes from trusted repo-local agents in `.codereview/agents/` on the PR base branch.\n\n") + if repo.RepoInfo == nil { + out.WriteString("Guidance provenance: unavailable.\n") + return out.String() + } + out.WriteString("Guidance provenance: ") + out.WriteString(repo.RepoInfo.Provenance) + out.WriteString("\n") + if source, ok := repoGuidanceSource(repo.Sources); ok { + out.WriteString("Guidance source status: ") + out.WriteString(string(source.Status)) + out.WriteString("\n") + switch source.Status { + case agents.SourceStatusAvailable: + out.WriteString("Base branch `.codereview/agents/` was loaded for this review.\n") + case agents.SourceStatusMissing: + out.WriteString("Base branch `.codereview/agents/` was not present for this review.\n") + case agents.SourceStatusUnreadable, agents.SourceStatusInvalid: + out.WriteString("Base branch `.codereview/agents/` could not be used as review guidance.\n") + if msg := strings.TrimSpace(source.Error); msg != "" { + out.WriteString("Source detail: ") + out.WriteString(msg) + out.WriteString("\n") + } } } + if note := strings.TrimSpace(repo.RepoInfo.TrustNote()); note != "" { + out.WriteString("\n") + out.WriteString(note) + out.WriteString("\n") + } + if repo.ExplicitReviewGuidance { + out.WriteString("\nAdditional explicit review guidance source: ") + out.WriteString(strings.TrimSpace(repo.ExplicitReviewGuidanceSource)) + out.WriteString("\n") + } else { + out.WriteString("\nSeparate review-guidance files are not used for this review; repo-local agents are the repo-owned guidance surface.\n") + } return out.String() } +func repoGuidanceSource(sources []agents.SourceInfo) (agents.SourceInfo, bool) { + for _, source := range sources { + if source.Kind == agents.SourceRepo { + return source, true + } + } + return agents.SourceInfo{}, false +} + func buildDossierIndex(dir string) (dossierIndexArtifact, error) { var files []dossierIndexFileArtifact root, err := os.OpenRoot(dir) diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go index dc26c90..65dec15 100644 --- a/internal/pipeline/pipeline_test.go +++ b/internal/pipeline/pipeline_test.go @@ -189,7 +189,9 @@ func TestDryRunPlansAndPersistsWithoutProviderWrites(t *testing.T) { assertFileContains(t, filepath.Join(result.Artifacts.DossierDir, "final", "discussion.md"), "main.go:2") assertFileContains(t, filepath.Join(result.Artifacts.DossierDir, "final", "discussion.md"), "Top-level concern") assertFileContains(t, filepath.Join(result.Artifacts.DossierDir, "final", "discussion.md"), "Review body") - assertFileContains(t, filepath.Join(result.Artifacts.DossierDir, "final", "repo-guidance.md"), "No dedicated repo review-guidance source is defined yet.") + assertFileContains(t, filepath.Join(result.Artifacts.DossierDir, "final", "repo-guidance.md"), "Guidance provenance: repo@refs/heads/main:") + assertFileContains(t, filepath.Join(result.Artifacts.DossierDir, "final", "repo-guidance.md"), "Guidance source status: missing") + assertFileContains(t, filepath.Join(result.Artifacts.DossierDir, "final", "repo-guidance.md"), "PR-head .codereview/agents changes do not affect this listing.") assertDossierIndexArtifact(t, result.Artifacts.DossierDir, "final/discussion.md") assertFileOmits(t, filepath.Join(result.Artifacts.DossierDir, "final", "discussion.md"), "provider_session_id", "session_row_id", "mergeability", "approval", "CI status", "Approved body should stay out of reviewer-facing discussion") assertFileContains(t, filepath.Join(result.Artifacts.DossierDir, "raw", "top-level-comments.json"), "Approved body should stay out of reviewer-facing discussion")