open-code-review/internal/diff
Gongyl01 0ce730a3c8
feat(manifest): run manifest coverage contract for review (#367) (#520)
* 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>
2026-08-01 16:50:21 +08:00
..
git.go feat(manifest): run manifest coverage contract for review (#367) (#520) 2026-08-01 16:50:21 +08:00
git_resolve_test.go feat(manifest): run manifest coverage contract for review (#367) (#520) 2026-08-01 16:50:21 +08:00
git_test.go fix: align Go module path with actual GitHub repository (#526) 2026-07-27 19:52:35 +08:00
gitignore.go feat: add ocr scan for full-file code review (#93) 2026-06-24 22:07:25 +08:00
gitignore_test.go fix(diff): honor .gitignore negation patterns (#651) 2026-08-01 11:12:02 +08:00
hunk.go style: apply go fmt formatting 2026-06-03 17:34:28 +08:00
hunk_test.go feat: init 2026-05-20 22:03:52 +08:00
parser.go refactor(diff): strip index headers from review prompts (#609) 2026-07-30 17:27:13 +08:00
parser_test.go refactor(diff): strip index headers from review prompts (#609) 2026-07-30 17:27:13 +08:00
relocation.go fix: align Go module path with actual GitHub repository (#526) 2026-07-27 19:52:35 +08:00
relocation_test.go fix: align Go module path with actual GitHub repository (#526) 2026-07-27 19:52:35 +08:00
resolver.go docs: remove stale Java-implementation references from code comments (#621) 2026-07-31 12:57:57 +08:00
resolver_test.go fix: align Go module path with actual GitHub repository (#526) 2026-07-27 19:52:35 +08:00
workspace_file.go fix: align Go module path with actual GitHub repository (#526) 2026-07-27 19:52:35 +08:00
workspace_file_test.go test(diff): add unit tests for readWorkspaceFileForDiff 2026-06-15 11:51:51 +08:00