mirror of
https://github.com/alibaba/open-code-review.git
synced 2026-08-31 10:35:56 +00:00
* feat(session): add run manifest coverage data model and builder First slice of issue #367 (run manifest coverage contract): the data model and state machine only. Not yet wired into the agent or CLI, so existing review/scan output is unchanged. Introduce the versioned, immutable RunManifest (schema ocr.run-manifest/v1) and a concurrency-safe ManifestBuilder that tracks per-file coverage (selected/completed/reused/failed/waived) and freezes into a terminal state. - terminal state derived solely from coverage sets, never comments/warnings (complete/partial/failed/skipped) - Finalize sweeps any undecided selected item to failed/unknown so no item is silently dropped - single-mutex builder: first terminal state wins, frozen after Finalize, nil-receiver safe - fixed failure classification enum with an unknown catch-all - redaction floor on failure/waive reasons (strip secrets, cap length) as a single write entry so callers cannot bypass it - 22 unit tests, race-clean Refs: issue #367 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(session): harden run manifest per adversarial review Address findings from the concurrency / JSON-contract / PR#306-coupling adversarial review of the manifest data model (still slice 1; not wired to agent or CLI). - SetSweepClass: Finalize can classify undispatched items as cancelled/budget instead of a blanket unknown (the one real model gap the review found) - ItemID(fingerprint)=SHA-256 canonical mint helper; an item_id is never a raw fingerprint, keeping the resume cross-reference explicit and mix-ups caught - sanitizeReason: strip control/ANSI chars, coerce valid UTF-8, redact quoted secret values, guarantee single line - Finalize returns deep-copied coverage slices so the frozen snapshot is never aliased across the two outlets - RegisterSelected: nil-safe (lazy-init map) + documents that only the post-deletion/post-filter dispatchable set may be registered +7 unit tests (29 total), race-clean. Refs: issue #367 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(manifest): wire input identity, config hashes and run-level failure (shard ②d) - Freeze per-mode input identity (mode + resolved_base/head + exact_range + source_artifact_sha256) via diff.ResolveInput/commitParents, and repository identity via RemoteIdentity/canonicalRemote (credential-free). - Add rule_config_sha256 and runtime_config_sha256 over an allowlist of non-secret fields using a length-prefixed SHA-256 framework (no tokens/URLs). - Replace SetRunLevelFailure(bool) with structured SetRunFailure(class, reason) and set ManifestInput.mode; fill execution.* (ocr version, provider, model, concurrency, config hashes). - Thread error returns through Finalize/WriteSessionEnd (main review path surfaces them; skip/all-failed/scan paths hardened in follow-up). - Tests: manifest_hash, canonical_config, git_resolve. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(manifest): propagate persistence errors and harden remote/error classification Merged review themes A/B/E from the 07-22 consolidated assessment. Theme A — Finalize / session_end delivery errors no longer swallowed: - agent.go no-files path returns the Finalize error instead of nil (A1) - agent.go loadDiffs failure joins the Finalize error via errors.Join (A2) - session.Finalize uses sync.Once + cached finalizeErr: written exactly once, concurrency-safe, and every caller replays the same result so a retry cannot falsely report success (A3) - scan/agent.go wires both Finalize call sites to surface the error (A4) Theme B — canonicalRemote rewritten (internal/diff/git.go): - keep the port (u.Host, not u.Hostname) so endpoints differing only by port stay distinct (B1) - split scp syntax on the first ':' so an '@' inside the path survives (B2) - recognize local/file/Windows/UNC remotes and omit identity rather than misparsing a path as a host (B3; local-remote policy still open) Theme E — main_task-empty is now a sentinel (errMainTaskEmpty) classified via errors.Is instead of matching error text. Theme D (TOCTOU) deferred to shard 4 per issue #367 open-issues OI-12. Tests: go build ./... + go vet + go test ./... all green (23 pkgs). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(manifest): report both dispatch and persistence errors on the normal path The success-path Finalize wiring used `ferr != nil && err == nil`, so when the review (or scan) failed AND session_end also failed to persist, the persistence error was dropped and only the dispatch error surfaced — the caller never learned the session/manifest was not saved. Join both with errors.Join when both occur (matching the loadDiffs path), so a persistence failure is always reported even alongside a dispatch failure. This closes the last gap in the OI-10 contract. - internal/agent/agent.go: review normal path - internal/scan/agent.go: scan normal path (+ errors import) Tests: go build ./... + go vet + go test ./... all green (23 pkgs). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(manifest): 接入 CLI 与 viewer 并补齐验收用例 - 使用冻结 manifest 统一 review JSON、文本与退出状态\n- session CLI 和 viewer 展示五集合覆盖并兼容 legacy/aborted\n- 补充本地 mock、跨出口一致性及安全验收用例 * test(manifest): 补齐验收矩阵缺口并修复审核发现的缺陷 验收用例:configuration 分类(run 级 sweep + item 级映射)、budget/timeout/panic 混合 partial 隔离、跨出口一致性改为规范化原始字节比对、flag 校验失败无产物断言。 代码修复:sanitizeReason 先剥控制字符再脱敏(堵控制字节绕过)、失败项异分类二次标记报冲突错误、source_artifact_sha256 按 item_id 去重并稳定排序、sortItems 改 SliceStable 对齐设计用词。 全仓 go test 23 包通过。 * test(manifest): 补充 provider transition resume 测试用例 覆盖 issue #367 验收标准 provider transition:resume 时 provider/model 改变后,子 manifest 记录当前值而非继承父运行,并经 parent_run_id 链接父会话以支持审计。用 mock client,不依赖真实 provider key。 * fix(manifest): 对齐预算终态与持久化语义 统一聚合预算停止时的 coverage、status 与退出码。传播 session writer 初始化错误,并补齐 merge first-parent 输入身份及回归测试。移除代码注释中的外部设计文档引用。 --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: kite <254839944+lizhengfeng101@users.noreply.github.com>
272 lines
8 KiB
Go
272 lines
8 KiB
Go
package session
|
|
|
|
import (
|
|
"path/filepath"
|
|
"testing"
|
|
"time"
|
|
|
|
"github.com/alibaba/open-code-review/internal/model"
|
|
)
|
|
|
|
func TestListSessions_EmptyRepoReturnsNil(t *testing.T) {
|
|
tmpHome := t.TempDir()
|
|
t.Setenv("HOME", tmpHome)
|
|
|
|
got, err := ListSessions(t.TempDir())
|
|
if err != nil {
|
|
t.Fatalf("ListSessions: %v", err)
|
|
}
|
|
if len(got) != 0 {
|
|
t.Errorf("expected empty result, got %d entries", len(got))
|
|
}
|
|
}
|
|
|
|
func TestListSessions_SortsAndAggregates(t *testing.T) {
|
|
tmpHome := t.TempDir()
|
|
t.Setenv("HOME", tmpHome)
|
|
repoDir := t.TempDir()
|
|
|
|
older := writeTestSession(t, repoDir, "feature-a", "commit-x", []model.LlmComment{
|
|
{Path: "a.go", Content: "one"},
|
|
}, 1, 0, true)
|
|
time.Sleep(1100 * time.Millisecond)
|
|
newer := writeTestSession(t, repoDir, "feature-a", "commit-y", []model.LlmComment{
|
|
{Path: "b.go", Content: "one"},
|
|
{Path: "b.go", Content: "two"},
|
|
}, 2, 1, false)
|
|
|
|
got, err := ListSessions(repoDir)
|
|
if err != nil {
|
|
t.Fatalf("ListSessions: %v", err)
|
|
}
|
|
if len(got) != 2 {
|
|
t.Fatalf("expected 2 sessions, got %d", len(got))
|
|
}
|
|
if got[0].SessionID != newer {
|
|
t.Errorf("expected newest first, got %q vs %q", got[0].SessionID, newer)
|
|
}
|
|
if got[1].SessionID != older {
|
|
t.Errorf("expected older second, got %q", got[1].SessionID)
|
|
}
|
|
|
|
if !got[0].Aborted {
|
|
t.Errorf("newest session was interrupted; expected Aborted=true")
|
|
}
|
|
if got[1].Aborted {
|
|
t.Errorf("older session was finalized; expected Aborted=false")
|
|
}
|
|
if got[0].TotalComments != 2 {
|
|
t.Errorf("newest TotalComments = %d, want 2", got[0].TotalComments)
|
|
}
|
|
if got[0].FailedFiles != 1 {
|
|
t.Errorf("newest FailedFiles = %d, want 1", got[0].FailedFiles)
|
|
}
|
|
if got[0].CompletedFiles != 2 {
|
|
t.Errorf("newest CompletedFiles = %d, want 2", got[0].CompletedFiles)
|
|
}
|
|
}
|
|
|
|
func TestLoadDetail_ReturnsItems(t *testing.T) {
|
|
tmpHome := t.TempDir()
|
|
t.Setenv("HOME", tmpHome)
|
|
repoDir := t.TempDir()
|
|
|
|
sh := New(repoDir, "main", "test-model", SessionOptions{
|
|
ReviewMode: ReviewModeCommit,
|
|
DiffCommit: "abc123",
|
|
})
|
|
sh.RecordReviewItemDone("a.go", "a.go", "a.go", "fp-a", []model.LlmComment{{Path: "a.go", Content: "note"}})
|
|
sh.RecordReviewItemReused("b.go", "b.go", "b.go", "fp-b", "prior-session", []model.LlmComment{{Path: "b.go", Content: "cached"}})
|
|
sh.RecordReviewItemFailed("c.go", "c.go", "c.go", "fp-c", "boom")
|
|
sh.Finalize()
|
|
|
|
summary, items, err := LoadDetail(repoDir, sh.SessionID)
|
|
if err != nil {
|
|
t.Fatalf("LoadDetail: %v", err)
|
|
}
|
|
if summary.CompletedFiles != 1 || summary.ReusedFiles != 1 || summary.FailedFiles != 1 {
|
|
t.Fatalf("summary = %+v", summary)
|
|
}
|
|
if summary.TotalComments != 2 {
|
|
t.Errorf("TotalComments = %d, want 2", summary.TotalComments)
|
|
}
|
|
if summary.Aborted {
|
|
t.Errorf("summary should not be aborted after Finalize")
|
|
}
|
|
if len(items) != 3 {
|
|
t.Fatalf("expected 3 items, got %d", len(items))
|
|
}
|
|
byType := map[string]ItemDetail{}
|
|
for _, it := range items {
|
|
byType[it.Type] = it
|
|
}
|
|
if reused := byType["reused"]; reused.SourceSessionID != "prior-session" {
|
|
t.Errorf("reused source = %q, want prior-session", reused.SourceSessionID)
|
|
}
|
|
if failed := byType["failed"]; failed.Error != "boom" {
|
|
t.Errorf("failed error = %q, want boom", failed.Error)
|
|
}
|
|
if done := byType["done"]; done.Comments != 1 {
|
|
t.Errorf("done comments = %d, want 1", done.Comments)
|
|
}
|
|
}
|
|
|
|
func TestLoadSummary_FallsBackToSessionEndFilesReviewed(t *testing.T) {
|
|
tmpHome := t.TempDir()
|
|
t.Setenv("HOME", tmpHome)
|
|
repoDir := t.TempDir()
|
|
|
|
sh := New(repoDir, "main", "test-model", SessionOptions{
|
|
ReviewMode: ReviewModeWorkspace,
|
|
})
|
|
sh.GetOrCreateFileSession("legacy-a.go")
|
|
sh.GetOrCreateFileSession("legacy-b.go")
|
|
sh.Finalize()
|
|
|
|
summary, items, err := LoadDetail(repoDir, sh.SessionID)
|
|
if err != nil {
|
|
t.Fatalf("LoadDetail: %v", err)
|
|
}
|
|
if summary.CompletedFiles != 2 {
|
|
t.Fatalf("CompletedFiles = %d, want 2", summary.CompletedFiles)
|
|
}
|
|
if summary.ReusedFiles != 0 || summary.FailedFiles != 0 {
|
|
t.Fatalf("unexpected checkpoint counts: %+v", summary)
|
|
}
|
|
if len(items) != 0 {
|
|
t.Fatalf("legacy session should not synthesize item details, got %d", len(items))
|
|
}
|
|
if !summary.Legacy || summary.RunManifest != nil {
|
|
t.Fatalf("legacy summary flags = legacy:%v manifest:%v", summary.Legacy, summary.RunManifest)
|
|
}
|
|
}
|
|
|
|
func TestLoadSummaryPrefersV1RunManifest(t *testing.T) {
|
|
tmpHome := t.TempDir()
|
|
t.Setenv("HOME", tmpHome)
|
|
repoDir := t.TempDir()
|
|
|
|
sh := New(repoDir, "main", "test-model", SessionOptions{
|
|
ReviewMode: ReviewModeWorkspace,
|
|
Operation: OperationReview,
|
|
})
|
|
b := sh.Manifest()
|
|
b.SetInput(ManifestInput{Mode: InputModeWorkspace})
|
|
items := []CoverageItem{
|
|
{ItemID: "a", Path: "a.go"},
|
|
{ItemID: "b", Path: "b.go"},
|
|
{ItemID: "c", Path: "c.go"},
|
|
{ItemID: "d", Path: "d.go"},
|
|
}
|
|
for _, item := range items {
|
|
if err := b.RegisterSelected(item); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
}
|
|
if err := b.SealSelected(); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
if err := b.MarkCompleted("a"); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
if err := b.MarkReused("b"); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
if err := b.MarkFailed("c", FailureProvider, "provider request failed"); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
if err := b.MarkWaived("d", "accepted by user"); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
manifest, err := b.Finalize(time.Second)
|
|
if err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
sh.SetFinalManifest(&manifest)
|
|
if err := sh.Finalize(); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
|
|
summary, err := LoadSummary(repoDir, sh.SessionID)
|
|
if err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
if summary.Legacy || summary.Aborted || summary.RunManifest == nil {
|
|
t.Fatalf("summary flags = legacy:%v aborted:%v manifest:%v", summary.Legacy, summary.Aborted, summary.RunManifest)
|
|
}
|
|
if summary.RunManifest.TerminalState != StatePartial {
|
|
t.Fatalf("terminal_state = %q", summary.RunManifest.TerminalState)
|
|
}
|
|
if summary.SelectedFiles != 4 || summary.CompletedFiles != 1 || summary.ReusedFiles != 1 || summary.FailedFiles != 1 || summary.WaivedFiles != 1 {
|
|
t.Fatalf("coverage counts = %+v", summary)
|
|
}
|
|
}
|
|
|
|
func TestLoadSummaryIgnoresUnknownManifestVersion(t *testing.T) {
|
|
tmpHome := t.TempDir()
|
|
t.Setenv("HOME", tmpHome)
|
|
repoDir := t.TempDir()
|
|
|
|
sh := New(repoDir, "main", "test-model", SessionOptions{ReviewMode: ReviewModeWorkspace})
|
|
sh.SetFinalManifest(&RunManifest{SchemaVersion: "ocr.run-manifest/v999", TerminalState: StateComplete})
|
|
if err := sh.Finalize(); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
summary, err := LoadSummary(repoDir, sh.SessionID)
|
|
if err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
if !summary.Legacy || summary.RunManifest != nil {
|
|
t.Fatalf("unknown schema must be treated as legacy: %+v", summary)
|
|
}
|
|
}
|
|
|
|
func TestLoadSummary_MissingFile(t *testing.T) {
|
|
tmpHome := t.TempDir()
|
|
t.Setenv("HOME", tmpHome)
|
|
if _, err := LoadSummary(t.TempDir(), "nonexistent"); err == nil {
|
|
t.Fatal("expected error for missing session")
|
|
}
|
|
}
|
|
|
|
// writeTestSession creates a real JSONL session using the persistence layer
|
|
// so tests exercise the same on-disk format that ListSessions consumes.
|
|
// It returns the session id.
|
|
func writeTestSession(t *testing.T, repoDir, from, to string, comments []model.LlmComment, doneCount, failedCount int, finalize bool) string {
|
|
t.Helper()
|
|
sh := New(repoDir, "main", "test-model", SessionOptions{
|
|
ReviewMode: ReviewModeRange,
|
|
DiffFrom: from,
|
|
DiffTo: to,
|
|
})
|
|
for i := 0; i < doneCount; i++ {
|
|
filePath := filepath.Base(t.TempDir()) + ".go"
|
|
var perFile []model.LlmComment
|
|
if i < len(comments) {
|
|
perFile = []model.LlmComment{comments[i]}
|
|
}
|
|
sh.RecordReviewItemDone(filePath, filePath, filePath, "fp-"+filePath, perFile)
|
|
}
|
|
for i := 0; i < failedCount; i++ {
|
|
filePath := "failed-" + filepath.Base(t.TempDir()) + ".go"
|
|
sh.RecordReviewItemFailed(filePath, filePath, filePath, "fp-fail-"+filePath, "test error")
|
|
}
|
|
if finalize {
|
|
sh.Finalize()
|
|
} else {
|
|
// Simulate an aborted run: flush the writer without emitting session_end.
|
|
if sh.persist != nil {
|
|
sh.persist.mu.Lock()
|
|
if sh.persist.writer != nil {
|
|
sh.persist.writer.Flush()
|
|
}
|
|
if sh.persist.file != nil {
|
|
_ = sh.persist.file.Close()
|
|
}
|
|
sh.persist.writer = nil
|
|
sh.persist.file = nil
|
|
sh.persist.mu.Unlock()
|
|
}
|
|
}
|
|
return sh.SessionID
|
|
}
|