diff --git a/doc/architecture/SPEC/engine/executor.md b/doc/architecture/SPEC/engine/executor.md index 9bf26293a..77084dddb 100644 --- a/doc/architecture/SPEC/engine/executor.md +++ b/doc/architecture/SPEC/engine/executor.md @@ -1,6 +1,6 @@ # engine/executor.py —— 规范 - + `LOC ~293 · 引擎底座（那个循环） · refactor-status: ok` @@ -22,6 +22,8 @@ helper：`_eval_when`、`_merge`。 - **B2**：resume 在跳过之前先恢复 `outputs.state_updates`；成功后 `state.update(state_updates)` 并按 step id 索引输出。 - `_merge` 把每个 foreach 条目的 `state_updates` **提升**到顶层（同键最后写者胜）。 +- `foreach` 中会写工作区、推送或写知识库的 step 顺序执行，共享 checkout 不并发修改； + 只读 fan-out 仍可并发执行。 - **B3**：`when:` 先读 TaskSpec 再读 state；**未知键 → 阻塞，绝不静默**。 - **B1**：类型化路由；未处理的异常 → BLOCKED（**绝不吞掉**）。 - 只对 RETRYABLE 重试，受 `max_step_retries` 限制。 diff --git a/doc/architecture/SPEC/engine/steps/pr.md b/doc/architecture/SPEC/engine/steps/pr.md index e48700ebc..c104694b4 100644 --- a/doc/architecture/SPEC/engine/steps/pr.md +++ b/doc/architecture/SPEC/engine/steps/pr.md @@ -1,6 +1,6 @@ # engine/steps/pr/ —— 规范 - + `LOC ~1390（6 个文件） · step 库（PR） · refactor-status: ok` @@ -26,6 +26,14 @@ `pr.rebase_onto_base`、`agent.debug_group`（agent/write_workspace）； `agent.verify_module`（validation/read）；`pr.post_review`（script/push）。 +`pr-debug` 的 report-only 路径不 checkout PR 分支。诊断组数受 playbook 参数和 +settings 安全上限共同约束；执行组共享 checkout 时必须顺序修改。`agent.debug_group` +只有在观察到新 commit、干净的 tracked checkout、根因和验证证据时才算成功。 +`agent.verify_module` 的咨询结论是 `verified | problem | inconclusive`，未知文本 +不能标成已验证。`ci.push` 对 PR debug/rebase 还需同一 HEAD 的 patch-gate 批准。 +知识 intake 的 canonical 记录使用 schema v2、完整仓库名和稳定 event ID；缺少 +完整身份时保留 v1 兼容记录，由消费者显式解析别名。 + ## 公开契约（可从 `engine.steps.pr` import） `extract_signature`（由 `utils` 再导出；供测试使用）。 diff --git a/doc/architecture/SPEC/engine/steps/review.md b/doc/architecture/SPEC/engine/steps/review.md index 10fb576a1..9c56e3be9 100644 --- a/doc/architecture/SPEC/engine/steps/review.md +++ b/doc/architecture/SPEC/engine/steps/review.md @@ -1,12 +1,15 @@ # engine/steps/review/ —— 规范 - + `LOC ~900（6 个文件） · step 库（评审） · refactor-status: ok` ## 职责 条件式 patch 门 + PR 评审 agent step + 有界的 PR review-readiness 质量 step， 以及它们仓库中立的 prompt 体系。 +Pre-push patch 门必须审查显式 base 到当前 HEAD 的已提交变更；空范围、脏的 +tracked checkout、不可解析的 ref 或超过 reviewer 容量的 diff 都阻塞推送。 +通过后记录批准的 base/head，后续推送必须验证当前 HEAD 仍匹配。 它曾是一个 341 行的模块；现在是一个把评测调优过的 prompt 数据、handler、 确定性 helper 三者分开的包。 diff --git a/doc/architecture/SPEC/playbooks/PLAYBOOKS.md b/doc/architecture/SPEC/playbooks/PLAYBOOKS.md index 8f26d769d..112612993 100644 --- a/doc/architecture/SPEC/playbooks/PLAYBOOKS.md +++ b/doc/architecture/SPEC/playbooks/PLAYBOOKS.md @@ -1,6 +1,6 @@ # playbooks/*.yaml —— 规范 - + `9 个文件 · 声明式编排数据 · refactor-status: ok` @@ -23,6 +23,8 @@ success, steps[]`。 **不要改它的 step 列表**。 - `pr-rebase`/`pr-debug`/`pr-review`/`pr-quality`/`issue-answer`/`issue-triage` —— active， 仓库中立（`repos: []`、`requires: [repo.path]`）。 +- `pr-debug` 的 report-only 模式跳过 checkout 和 push；修复模式在推送前审查 + 已提交的变更。 - `repo-profile` —— active，仓库中立（用于接入第二个仓库）。 - `profile-consolidate` —— **candidate** （planner 不可见；只能经 `--playbook` 运行）。 diff --git a/doc/architecture/SPEC/playbooks/store.md b/doc/architecture/SPEC/playbooks/store.md index 71de6caee..6c24e5a5a 100644 --- a/doc/architecture/SPEC/playbooks/store.md +++ b/doc/architecture/SPEC/playbooks/store.md @@ -1,6 +1,6 @@ # playbooks/store.py —— 规范 - + `LOC ~166 · 规划（注册表） · refactor-status: ok` @@ -21,6 +21,7 @@ locked > active；高版本 > 低版本。 - `validate` 拒绝引用了未注册 step 的 playbook（**加载即失败**）。 - `save_candidate` 强制 `status=candidate`（**D1** —— 不允许自我晋升）。 +- `playbook_to_doc` 保留 `mode_aware`，candidate 的序列化/解析往返不能改变模式门。 ## 边界 —— 不属于这里 不执行、不含规划策略（那是 planner 的）、不含 step 逻辑。 diff --git a/doc/architecture/SPEC/profiles/store.md b/doc/architecture/SPEC/profiles/store.md index 4bafa3907..b0e7f4f02 100644 --- a/doc/architecture/SPEC/profiles/store.md +++ b/doc/architecture/SPEC/profiles/store.md @@ -1,6 +1,6 @@ # profiles/store.py —— 规范 - + `LOC ~268 · profile（精选层） · refactor-status: ok` @@ -16,6 +16,8 @@ ## 不变量 - op 是**唯一**的变更路径；畸形/被禁的 op **逐条**拒绝（绝不抛异常）；档位不对的 op 被拒（**D4**）。 +- 未知 tier、非 object op、非 `list[str]` 的证据均只拒绝对应 op；同批后续合法 op + 仍可应用。 - `add_fact` 需要正文 + 证据；重复 id 视为一次**确认**（**D3**）。 - `rewrite_fact` 绝不让某条 fact 变成无证据；**稳定**（≥3 次确认）的 fact 不得丢弃已 引用的证据；被取代的正文进 `history`（**D3**）。 diff --git a/doc/architecture/SPEC/review/change_set.md b/doc/architecture/SPEC/review/change_set.md new file mode 100644 index 000000000..416cdd9db --- /dev/null +++ b/doc/architecture/SPEC/review/change_set.md @@ -0,0 +1,16 @@ +# review/change_set.py —— 规范 + + + +`ChangeSet.capture(repo, base_ref)` freezes the committed range that a PR +mutation workflow intends to push. The base must be explicit and resolve to a +commit. `HEAD` is resolved once; the returned `(base_sha, head_sha, diff_text)` +is immutable. The diff includes binary changes and disables external diff +drivers. A tracked dirty checkout, unresolved ref, or failed diff raises +`ChangeSetError` so the pre-push review blocks rather than approving an +uninspected change. Untracked files are not part of a commit and are excluded. + +The patch gate rejects an empty committed range and diffs over its reviewer +budget, then records approval for the exact base and head only after a passing +review. `ci.push` compares the approved head with the current checkout head +before any PR debug/rebase push. A later commit requires another review. diff --git a/doc/architecture/SPEC/sdk.md b/doc/architecture/SPEC/sdk.md index b6e76c2ed..6c2639d79 100644 --- a/doc/architecture/SPEC/sdk.md +++ b/doc/architecture/SPEC/sdk.md @@ -1,6 +1,6 @@ # sdk/ —— 规范 - + `Python SDK v1 · 跨仓库唯一 typed 边界 · refactor-status: ok` @@ -54,8 +54,9 @@ - **Strict findings 绑定发布结果**：`get_result` 的 `findings` 对最终发布集合逐条返回 `finding_id`、最终 `severity`、`anchor` 与 `head_sha`，保持顺序及重复 identity。 identity 以文件路径与完整归一化文本生成，行号变动不改变 identity；它不与 - `finding_dispositions` 按 anchor 拼接。当前没有 carried-finding recheck，缺失旧 - finding 不代表已修复；不得承诺 `head_recheck` 或 `findings_missing_carried` 字段。 + `finding_dispositions` 按 anchor 拼接。Direct 1.1 / Strict 1.3 对传入的 + carried finding 返回独立、绑定当前 head 的显式 `finding_rechecks`；缺失或 + `unverified` 的答案不代表已修复，详见下方 carried-finding recheck 契约。 - **知识规则由 provider 唯一定义**：catalog 只暴露当前仓库 owner 与 general 的 owner rule page document ID —— 每个 owner 的 `rules.md` 入口页及其 `type: rule` 的 `rules-.md` 专题页（`catalog_entries()` 附每页 diff --git a/playbooks/pr-debug.yaml b/playbooks/pr-debug.yaml index f7cd59f68..466314484 100644 --- a/playbooks/pr-debug.yaml +++ b/playbooks/pr-debug.yaml @@ -14,7 +14,7 @@ params: max_groups: {type: int, default: 6} steps: - {id: guard, step: workspace.guard_clean} - - {id: checkout, step: pr.checkout_branch, params: {force_push: false}} + - {id: checkout, step: pr.checkout_branch, params: {force_push: false}, when: not report_only} - {id: fetch_ci, step: pr.fetch_ci_failures} - {id: group, step: pr.group_failures} - {id: debug, step: agent.debug_group, foreach: failure_groups, when: not report_only} diff --git a/src/infermatrix_copilot/engine/executor.py b/src/infermatrix_copilot/engine/executor.py index 5751f9756..b3c5fb6f2 100644 --- a/src/infermatrix_copilot/engine/executor.py +++ b/src/infermatrix_copilot/engine/executor.py @@ -157,10 +157,16 @@ async def run(self, playbook: "Playbook", state: dict) -> RunOutcome: # safety-bearing (`force_push`, `pre_push`) — so they override the # run-scoped ones rather than the other way round. step_params = {**task_params, **pstep.params} - results = await asyncio.gather( - *(self._run_step(spec, step_params, state, item, pstep.id) - for item in items) - ) + if spec.risk in {"write_workspace", "push", "knowledge"}: + # Foreach items share one checkout and run state. A writer must + # finish before the next item can edit or commit that checkout. + results = [await self._run_step(spec, step_params, state, item, pstep.id) + for item in items] + else: + results = await asyncio.gather( + *(self._run_step(spec, step_params, state, item, pstep.id) + for item in items) + ) result = _merge(results) outcome.step_results[pstep.id] = result self.trace.record( diff --git a/src/infermatrix_copilot/engine/steps/pr/debug.py b/src/infermatrix_copilot/engine/steps/pr/debug.py index 31f8bb363..1df39742a 100644 --- a/src/infermatrix_copilot/engine/steps/pr/debug.py +++ b/src/infermatrix_copilot/engine/steps/pr/debug.py @@ -17,6 +17,7 @@ from ...step import FailureKind, StepContext, StepResult from .._common import from_state, published, record_debug_memory, require_repo, step from .._common import gh as _gh +from .._common import git as _git from .._common import repo_path as _repo_path from .._common import task_spec as _task_spec from .utils import extract_signature @@ -117,7 +118,17 @@ async def _pr_group_failures(ctx: StepContext) -> StepResult: if f.get("log") and not g["log_excerpt"]: g["log_excerpt"] = f["log"][-4_000:] group_list = list(groups.values()) - max_groups = ctx.settings.pr_debug_max_groups + requested = ctx.params.get("max_groups") + try: + max_groups = (ctx.settings.pr_debug_max_groups if requested is None + else int(requested)) + except (TypeError, ValueError): + return StepResult(False, FailureKind.BLOCKED, + "max_groups must be a positive integer") + if max_groups < 1: + return StepResult(False, FailureKind.BLOCKED, + "max_groups must be a positive integer") + max_groups = min(max_groups, ctx.settings.pr_debug_max_groups) if len(group_list) > max_groups: return StepResult(False, FailureKind.ESCALATE, f"{len(group_list)} distinct failure groups exceeds the " @@ -154,6 +165,10 @@ async def _pr_debug_group(ctx: StepContext) -> StepResult: repo = require_repo(ctx, must_exist=False) if isinstance(repo, StepResult): return repo + before_rc, before_head = _git(repo, "rev-parse", "HEAD") + if before_rc != 0: + return StepResult(False, FailureKind.BLOCKED, + f"cannot identify checkout head before debug: {before_head[:300]}") from ...agent_runtime import run_agent_step result, output = await run_agent_step( @@ -172,6 +187,14 @@ async def _pr_debug_group(ctx: StepContext) -> StepResult: max_iters=ctx.settings.max_agent_iters, ) if result.ok: + after_rc, after_head = _git(repo, "rev-parse", "HEAD") + dirty_rc, dirty = _git(repo, "status", "--porcelain", "--untracked-files=no") + if (after_rc != 0 or dirty_rc != 0 or before_head == after_head + or dirty.strip() or not str(output.get("root_cause") or "").strip() + or not str(output.get("verification") or "").strip()): + return StepResult(False, FailureKind.BLOCKED, + f"'{sig}': fix needs an observed commit, clean tracked " + "checkout, root cause and verification") result.summary = (f"'{sig}': {output.get('fix_summary', '')[:150]} " f"(verified: {output.get('verification', '?')[:80]})") if output.get("root_cause") and output.get("verification"): @@ -234,17 +257,25 @@ async def _pr_harvest_debug_knowledge(ctx: StepContext) -> StepResult: return StepResult(True, summary="no verified fixes to harvest") spec = ctx.state.get("task_spec") or {} alias = str(spec.get("repo", "")) + full_name = ctx.settings.repo_full_names.get(alias, alias) + # Older installations may only know a local alias. Keep their v1 record + # readable, but never claim a canonical identity that was not configured. + canonical = isinstance(full_name, str) and full_name.count("/") == 1 record = { + "schema_version": 2 if canonical else 1, "run_id": ctx.run_dir.name, # Full GitHub identity when configured, so the consumer keys the # event correctly; the local alias otherwise. - "repo": ctx.settings.repo_full_names.get(alias, alias), + "repo": full_name, "pr": spec.get("pr"), "kind": "bugfix_run", "title": f"pr_debug fixes for PR #{spec.get('pr')}", "groups": fixes, "created_at": datetime.now(timezone.utc).isoformat(), } + if canonical: + record["event_id"] = f"bugfix_run:{full_name.casefold()}:{ctx.run_dir.name}" + record["repository"] = {"full_name": full_name, "alias": alias} # Two independent sinks: the local drop (same-host consumer) and the # mailbox issue (cross-host consumer). Each is attempted when # configured, and neither one's failure suppresses the other. @@ -285,7 +316,7 @@ async def _pr_harvest_debug_knowledge(ctx: StepContext) -> StepResult: outputs=outputs) -BUGFIX_RECORD_MARKER = "" +BUGFIX_RECORD_MARKER = "" _INTAKE_ISSUE = re.compile(r"^(?P[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+)#(?P[1-9][0-9]*)$") diff --git a/src/infermatrix_copilot/engine/steps/pr/publish.py b/src/infermatrix_copilot/engine/steps/pr/publish.py index a6262fa3c..9f44817ce 100644 --- a/src/infermatrix_copilot/engine/steps/pr/publish.py +++ b/src/infermatrix_copilot/engine/steps/pr/publish.py @@ -46,6 +46,17 @@ async def _push(ctx: StepContext) -> StepResult: decision = guard_push(policy, list(protected)) if not decision.allowed: return StepResult(False, FailureKind.FORBIDDEN, decision.reason) + if _task_spec(ctx).get("kind") in {"pr_debug", "pr_rebase"}: + from ....review.change_set import ChangeSet, ChangeSetError + + approval = ctx.state.get("approved_change_set") or {} + try: + current = ChangeSet.capture(repo, str(approval.get("base_sha") or "")) + except ChangeSetError as exc: + return StepResult(False, FailureKind.FORBIDDEN, f"pre-push approval unavailable: {exc}") + if current.head_sha != approval.get("head_sha"): + return StepResult(False, FailureKind.FORBIDDEN, + "PR head changed after patch review; review the new commit") if not ctx.settings.allow_push: return StepResult(True, summary=f"dry-run (ALLOW_PUSH=0): {' '.join(decision.command)}", outputs={"dry_run": True, "command": list(decision.command)}) diff --git a/src/infermatrix_copilot/engine/steps/pr/rebase.py b/src/infermatrix_copilot/engine/steps/pr/rebase.py index f14960aba..cb897bfc1 100644 --- a/src/infermatrix_copilot/engine/steps/pr/rebase.py +++ b/src/infermatrix_copilot/engine/steps/pr/rebase.py @@ -64,6 +64,9 @@ async def _pr_checkout(ctx: StepContext) -> StepResult: rc, out = _git(repo, "checkout", "-B", local, "FETCH_HEAD") if rc != 0: return StepResult(False, FailureKind.BLOCKED, f"checkout failed: {out[:400]}") + rc, initial_head_sha = _git(repo, "rev-parse", "HEAD") + if rc != 0: + return StepResult(False, FailureKind.BLOCKED, f"cannot identify PR head: {initial_head_sha[:400]}") force = bool(ctx.params.get("force_push", False)) policy = PushPolicy( @@ -72,7 +75,8 @@ async def _pr_checkout(ctx: StepContext) -> StepResult: ) ctx.state.update( pr_head_ref=head_ref, pr_head_remote=remote, pr_base_branch=base, - pr_local_branch=local, push_policy=policy, + pr_local_branch=local, pr_initial_head_sha=initial_head_sha, + push_policy=policy, ) return StepResult(True, summary=f"checked out PR #{pr} ({remote}/{head_ref} -> {local})", outputs={"local_branch": local, "base": base, "remote": remote, @@ -81,6 +85,7 @@ async def _pr_checkout(ctx: StepContext) -> StepResult: "state_updates": { "pr_head_ref": head_ref, "pr_head_remote": remote, "pr_base_branch": base, "pr_local_branch": local, + "pr_initial_head_sha": initial_head_sha, "push_policy": asdict(policy), }}) @@ -247,6 +252,12 @@ async def _verify_module(ctx: StepContext) -> StepResult: input_tokens=reply.usage.get("input_tokens", 0), output_tokens=reply.usage.get("output_tokens", 0)) text = reply.text.strip() - if text.upper().startswith("PROBLEM"): - return StepResult(False, FailureKind.REPLAN, f"{module}: {text[:400]}") - return StepResult(True, summary=f"{module}: verified") + if text.upper().startswith("PROBLEM:"): + return StepResult(False, FailureKind.REPLAN, f"{module}: {text[:400]}", + outputs={"verification_status": "problem"}) + if text.upper() == "OK" or text.upper().startswith("OK:"): + return StepResult(True, summary=f"{module}: verified", + outputs={"verification_status": "verified"}) + return StepResult(True, summary=f"{module}: advisory result inconclusive", + outputs={"verification_status": "inconclusive", + "advisory_reply": text[:400]}) diff --git a/src/infermatrix_copilot/engine/steps/review/steps.py b/src/infermatrix_copilot/engine/steps/review/steps.py index 50804119c..a37d5dcfc 100644 --- a/src/infermatrix_copilot/engine/steps/review/steps.py +++ b/src/infermatrix_copilot/engine/steps/review/steps.py @@ -22,6 +22,7 @@ from ....llm import parse_json_reply from ....review.diff_summary import build_diff_summary +from ....review.change_set import ChangeSet, ChangeSetError from ....review.planner import DEFAULT_STANDARD_LENSES, DEPTHS, plan_review from ....review.reviewer import run_patch_review from ....review.triggers import evaluate_triggers @@ -64,17 +65,36 @@ async def _patch_gate(ctx: StepContext) -> StepResult: repo = _repo_path(ctx) if repo is None: return StepResult(False, FailureKind.BLOCKED, "no repo path") + pre_push = bool(ctx.params.get("pre_push", False)) + change_set = None + base_ref = str(ctx.params.get("base_ref") or "HEAD") + if pre_push: + base_ref = str( + ctx.params.get("base_ref") or ctx.state.get("rebase_base_sha") + or ctx.state.get("pr_initial_head_sha") or "" + ) + try: + change_set = ChangeSet.capture(repo, base_ref) + except ChangeSetError as exc: + return StepResult(False, FailureKind.BLOCKED, str(exc)) + if not change_set.diff_text: + return StepResult(False, FailureKind.BLOCKED, + "no committed changes in the pre-push review range") + base_ref = change_set.base_sha summary = build_diff_summary( - repo, base_ref=ctx.params.get("base_ref", "HEAD"), + repo, base_ref=base_ref, primary_files=tuple(ctx.state.get("primary_files", ())), trace=ctx.trace, ) if not summary.changed_files and not summary.full_file_writes: + if pre_push: + return StepResult(False, FailureKind.BLOCKED, + "no committed changes in the pre-push review range") return StepResult(True, summary="no diff to review", outputs={"fired": [], "verdict": "not_required"}) fired = evaluate_triggers( summary, ctx.settings, touched_modules=tuple(ctx.state.get("touched_modules", ())), - pre_push=bool(ctx.params.get("pre_push", False)), + pre_push=pre_push, knowledge_edit=bool(ctx.state.get("knowledge_edit", False)), high_risk_modules=ctx.state.get("high_risk_modules"), ) @@ -82,14 +102,31 @@ async def _patch_gate(ctx: StepContext) -> StepResult: if not fired: return StepResult(True, summary="no review triggers fired", outputs={"fired": [], "verdict": "not_required"}) - diff = subprocess.run(["git", "diff", ctx.params.get("base_ref", "HEAD")], - cwd=str(repo), capture_output=True, text=True, - encoding="utf-8", errors="replace", timeout=60).stdout + if change_set is not None: + diff = change_set.diff_text + if len(diff) > 60_000: + return StepResult(False, FailureKind.ESCALATE, + "committed diff exceeds the patch reviewer's 60000-character budget") + else: + diff_result = subprocess.run(["git", "diff", base_ref], + cwd=str(repo), capture_output=True, text=True, + encoding="utf-8", errors="replace", timeout=60) + if diff_result.returncode != 0: + return StepResult(False, FailureKind.BLOCKED, + f"cannot read patch diff: {diff_result.stderr[:300]}") + diff = diff_result.stdout verdict = run_patch_review(ctx.llm, diff_text=diff, summary=summary, fired_rules=fired, model=ctx.settings.reviewer) ctx.trace.record("patch_review", fired=fired, verdict=verdict.verdict, critiques=verdict.critiques) if verdict.passing: + if change_set is not None: + approval = {"base_sha": change_set.base_sha, + "head_sha": change_set.head_sha} + ctx.state["approved_change_set"] = approval + return StepResult(True, summary=f"patch review lgtm (rules: {fired})", + outputs={"fired": fired, "verdict": "lgtm", + "state_updates": {"approved_change_set": approval}}) return StepResult(True, summary=f"patch review lgtm (rules: {fired})", outputs={"fired": fired, "verdict": "lgtm"}) if verdict.verdict == "revise": diff --git a/src/infermatrix_copilot/playbooks/store.py b/src/infermatrix_copilot/playbooks/store.py index 984c58c3b..721bd50d7 100644 --- a/src/infermatrix_copilot/playbooks/store.py +++ b/src/infermatrix_copilot/playbooks/store.py @@ -63,6 +63,7 @@ def playbook_to_doc(pb: Playbook) -> dict: return { "name": pb.name, "version": pb.version, "status": pb.status, "task_kinds": pb.task_kinds, "repos": pb.repos, "params": pb.params, + **({"mode_aware": True} if pb.mode_aware else {}), **({"requires": pb.requires} if pb.requires else {}), "provenance": pb.provenance, "success": pb.success, "steps": [ diff --git a/src/infermatrix_copilot/profiles/store.py b/src/infermatrix_copilot/profiles/store.py index 1eb3f23b4..524e6f6d0 100644 --- a/src/infermatrix_copilot/profiles/store.py +++ b/src/infermatrix_copilot/profiles/store.py @@ -119,11 +119,24 @@ def apply_ops(self, ops: list[dict], *, tier: str = "run", actor: str = "agent") -> list[str]: """Apply typed ops; returns per-op rejection reasons ('' = applied). Malformed/forbidden ops are rejected individually, never raised.""" - allowed = RUN_OPS if tier == "run" else CONSOLIDATE_OPS + allowed = {"run": RUN_OPS, "consolidate": CONSOLIDATE_OPS}.get(tier) results: list[str] = [] applied: list[dict] = [] for op in ops: - kind = str((op or {}).get("op", "")) + if allowed is None: + results.append(f"unknown profile tier '{tier}'") + continue + if not isinstance(op, dict): + results.append("profile operation must be an object") + continue + kind = str(op.get("op", "")) + evidence = op.get("evidence") + if evidence is not None and ( + not isinstance(evidence, list) + or not all(isinstance(item, str) for item in evidence) + ): + results.append(f"op '{kind}' evidence must be a list of strings") + continue if kind not in allowed: results.append(f"op '{kind}' not allowed in tier '{tier}'") continue diff --git a/src/infermatrix_copilot/review/change_set.py b/src/infermatrix_copilot/review/change_set.py new file mode 100644 index 000000000..3cc3931a9 --- /dev/null +++ b/src/infermatrix_copilot/review/change_set.py @@ -0,0 +1,51 @@ +"""A frozen, committed Git range used by the PR patch and push gates.""" + +from __future__ import annotations + +import subprocess +from dataclasses import dataclass +from pathlib import Path + + +class ChangeSetError(ValueError): + """The intended committed change cannot be established safely.""" + + +def _git(repo: Path, *args: str) -> subprocess.CompletedProcess[str]: + try: + return subprocess.run( + ["git", *args], cwd=repo, capture_output=True, text=True, + encoding="utf-8", errors="replace", timeout=60, + ) + except (OSError, subprocess.SubprocessError) as exc: + raise ChangeSetError(f"git {' '.join(args)} failed: {exc}") from exc + + +@dataclass(frozen=True) +class ChangeSet: + base_sha: str + head_sha: str + diff_text: str + + @classmethod + def capture(cls, repo_path: str | Path, base_ref: str) -> "ChangeSet": + """Read exactly the commits a subsequent `git push HEAD` can publish.""" + repo = Path(repo_path) + if not base_ref: + raise ChangeSetError("pre-push review needs an explicit base ref") + dirty = _git(repo, "diff", "--quiet", "HEAD") + if dirty.returncode != 0: + if dirty.returncode == 1: + raise ChangeSetError("commit tracked workspace edits before pre-push review") + raise ChangeSetError(f"cannot inspect workspace: {dirty.stderr[:300]}") + resolved = [] + for ref in (base_ref, "HEAD"): + result = _git(repo, "rev-parse", "--verify", f"{ref}^{{commit}}") + if result.returncode != 0: + raise ChangeSetError(f"cannot resolve review ref {ref!r}: {result.stderr[:300]}") + resolved.append(result.stdout.strip()) + base_sha, head_sha = resolved + diff = _git(repo, "diff", "--no-ext-diff", "--binary", base_sha, head_sha) + if diff.returncode != 0: + raise ChangeSetError(f"cannot read committed diff: {diff.stderr[:300]}") + return cls(base_sha=base_sha, head_sha=head_sha, diff_text=diff.stdout) diff --git a/test/test_agent_runtime.py b/test/test_agent_runtime.py index 06a74070f..1fc65e0fc 100644 --- a/test/test_agent_runtime.py +++ b/test/test_agent_runtime.py @@ -198,11 +198,26 @@ def test_every_agent_step_goes_through_the_runtime(settings, trace, tmp_path, llm = ScriptedLLM([contract(review_comments=[], answer_draft="a", triage_table=[], root_cause="r", fix_summary="f", verification="v")]) + if step_name == "agent.debug_group": + import subprocess + + repo = tmp_path / "repo" + repo.mkdir() + for args in (("init", "-q"), ("config", "user.email", "t@e.c"), + ("config", "user.name", "t")): + subprocess.run(["git", *args], cwd=repo, check=True) + (repo / "source.py").write_text("value = 1\n") + subprocess.run(["git", "add", "source.py"], cwd=repo, check=True) + subprocess.run(["git", "commit", "-q", "-m", "base"], cwd=repo, check=True) + state = {**state, "repo_path": str(repo)} ctx = StepContext(settings=settings, state=state, params={}, run_dir=tmp_path / "run", trace=trace, llm=llm) ctx.item = {"signature": "sig", "jobs": []} # for debug_group result = asyncio.run(registry.get(step_name).handler(ctx)) - assert result.ok, f"{step_name}: {result.summary}" + if step_name == "agent.debug_group": + assert not result.ok and "observed commit" in result.summary + else: + assert result.ok, f"{step_name}: {result.summary}" ev = next(trace.events("agent_dispatch")) assert ev["permissions"]["tools"], step_name diff --git a/test/test_engine.py b/test/test_engine.py index 55abbc2a6..23f606622 100644 --- a/test/test_engine.py +++ b/test/test_engine.py @@ -42,6 +42,30 @@ def test_sequential_execution_and_outputs(env): assert outcome.step_results["b"].summary == "got 1" +def test_mutating_foreach_owns_shared_checkout_one_item_at_a_time(env): + registry, executor, _ = env + active = 0 + peak = 0 + seen = [] + + async def edit(ctx): + nonlocal active, peak + active += 1 + peak = max(peak, active) + await asyncio.sleep(0) + seen.append(ctx.item) + active -= 1 + return StepResult(True, summary="edited") + + registry.register(StepSpec("s.edit", "agent", "write_workspace", edit)) + result = asyncio.run(executor.run( + playbook([PlaybookStep("edit", "s.edit", foreach="groups")]), + {"groups": ["first", "second"]}, + )) + assert result.status == "done" + assert seen == ["first", "second"] and peak == 1 + + def test_task_params_reach_steps(env): """`--task-param K=V` lands on the TaskSpec; it must also reach a step's `ctx.params`, which previously only ever saw the playbook's step params.""" diff --git a/test/test_knowledge_harvest.py b/test/test_knowledge_harvest.py index e425c1124..7c3bb9f67 100644 --- a/test/test_knowledge_harvest.py +++ b/test/test_knowledge_harvest.py @@ -80,6 +80,9 @@ def test_landed_fix_writes_one_record(registry, settings, trace, tmp_path): record = json.loads(path.read_text(encoding="utf-8")) assert record["run_id"] == "run-x" assert record["repo"] == "owner/demo" # full identity, not the alias + assert record["schema_version"] == 2 + assert record["event_id"] == "bugfix_run:owner/demo:run-x" + assert record["repository"] == {"full_name": "owner/demo", "alias": "demo"} assert record["pr"] == 42 assert record["kind"] == "bugfix_run" assert record["groups"][0]["signature"] == "ImportError: cycle" @@ -120,7 +123,7 @@ def test_landed_fix_is_posted_to_the_mailbox_issue( body = (tmp_path / "run-x" / "knowledge-intake-comment.md").read_text( encoding="utf-8" ) - assert body.startswith("\n```json\n") + assert body.startswith("\n```json\n") posted = json.loads(body.split("```json\n", 1)[1].rsplit("```", 1)[0]) # The comment carries exactly the record the drop file carries. dropped = json.loads((tmp_path / "intake" / "run-x.json").read_text(encoding="utf-8")) diff --git a/test/test_planner_playbooks.py b/test/test_planner_playbooks.py index dfe793714..e87d4996c 100644 --- a/test/test_planner_playbooks.py +++ b/test/test_planner_playbooks.py @@ -3,7 +3,7 @@ from infermatrix_copilot.engine.steps import register_builtin_steps from infermatrix_copilot.engine.planner import Planner, PlanningError from infermatrix_copilot.engine.registry import StepRegistry -from infermatrix_copilot.playbooks.store import PlaybookStore +from infermatrix_copilot.playbooks.store import PlaybookStore, parse_playbook, playbook_to_doc from infermatrix_copilot.task_spec import TaskSpec LOCKED_PB = """\ @@ -104,6 +104,15 @@ def test_candidate_save_roundtrip(stack, settings): assert store.find("pr_review") is None +def test_mode_aware_survives_playbook_serialization(): + original = parse_playbook({ + "name": "mode-aware", "status": "candidate", "version": 1, + "task_kinds": ["repo_rebase"], "mode_aware": True, + "steps": [{"id": "report", "step": "report.final_summary"}], + }) + assert parse_playbook(playbook_to_doc(original)).mode_aware is True + + def test_real_pr_review_playbook_reuses_with_review_depth(): """The shipped pr-review.yaml declares review_depth, so a spec carrying it resolves as reuse (L0) — an undeclared param would demote to adapt (L1, diff --git a/test/test_pr_steps.py b/test/test_pr_steps.py index c154ba2df..495b2c05c 100644 --- a/test/test_pr_steps.py +++ b/test/test_pr_steps.py @@ -8,7 +8,7 @@ from infermatrix_copilot.engine.steps import register_builtin_steps from infermatrix_copilot.engine.steps.pr import extract_signature from infermatrix_copilot.engine.registry import StepRegistry -from infermatrix_copilot.engine.step import FailureKind, StepContext +from infermatrix_copilot.engine.step import FailureKind, StepContext, StepResult from infermatrix_copilot.push import PushPolicy @@ -78,6 +78,87 @@ def test_checkout_report_only_disallows_push(registry, settings, trace, tmp_path assert state["push_policy"].allowed is False +def test_patch_gate_reviews_committed_range_and_push_binds_head( + registry, settings, trace, tmp_path, git_repo, monkeypatch, +): + from infermatrix_copilot.engine.steps.review import steps as review_steps + from infermatrix_copilot.review.reviewer import ReviewVerdict + + base = _git(git_repo, "rev-parse", "HEAD") + (git_repo / "mod_a.py").write_text("A = 2\n") + _git(git_repo, "add", "mod_a.py") + _git(git_repo, "commit", "-q", "-m", "fix committed change") + seen = [] + def approve(_llm, *, diff_text, **_kwargs): + seen.append(diff_text) + return ReviewVerdict("lgtm") + monkeypatch.setattr(review_steps, "run_patch_review", approve) + state = { + "repo_path": str(git_repo), "task_spec": {"kind": "pr_debug"}, + "pr_initial_head_sha": base, + "push_policy": PushPolicy(allowed=True, branch="feature"), + } + result = asyncio.run(registry.get("review.patch_gate").handler( + _ctx(settings, trace, tmp_path, state, params={"pre_push": True}) + )) + assert result.ok and seen and "+A = 2" in seen[0] + assert result.outputs["state_updates"]["approved_change_set"]["base_sha"] == base + push = registry.get("ci.push") + assert asyncio.run(push.handler(_ctx(settings, trace, tmp_path, state))).ok + + (git_repo / "mod_a.py").write_text("A = 3\n") + _git(git_repo, "add", "mod_a.py") + _git(git_repo, "commit", "-q", "-m", "changed after approval") + stale = asyncio.run(push.handler(_ctx(settings, trace, tmp_path, state))) + assert not stale.ok and "head changed" in stale.summary + + +def test_pre_push_patch_gate_requires_a_baseline_and_clean_tree( + registry, settings, trace, tmp_path, git_repo, +): + step = registry.get("review.patch_gate") + state = {"repo_path": str(git_repo), "task_spec": {"kind": "pr_debug"}} + missing = asyncio.run(step.handler(_ctx( + settings, trace, tmp_path, state, params={"pre_push": True} + ))) + assert not missing.ok and "explicit base" in missing.summary + + state["pr_initial_head_sha"] = _git(git_repo, "rev-parse", "HEAD") + empty = asyncio.run(step.handler(_ctx( + settings, trace, tmp_path, state, params={"pre_push": True} + ))) + assert not empty.ok and "no committed changes" in empty.summary + (git_repo / "mod_a.py").write_text("uncommitted\n") + dirty = asyncio.run(step.handler(_ctx( + settings, trace, tmp_path, state, params={"pre_push": True} + ))) + assert not dirty.ok and "commit tracked" in dirty.summary + + +@pytest.mark.parametrize( + ("reply", "status", "ok"), + [("OK", "verified", True), ("PROBLEM: dropped import", "problem", False), + ("probably fine", "inconclusive", True)], +) +def test_rebase_advisory_reports_explicit_status( + registry, settings, trace, tmp_path, git_repo, reply, status, ok, +): + from types import SimpleNamespace + + class LLM: + available = True + + def create(self, **_kwargs): + return SimpleNamespace(text=reply, usage={}) + + state = {"repo_path": str(git_repo), "rebase_base_sha": _git(git_repo, "rev-parse", "HEAD")} + result = asyncio.run(registry.get("agent.verify_module").handler( + _ctx(settings, trace, tmp_path, state, llm=LLM()) + )) + assert result.ok is ok + assert result.outputs["verification_status"] == status + + def test_clean_rebase_and_analyze(registry, settings, trace, tmp_path, pr_repos): origin, work = pr_repos # advance origin main with a NON-conflicting commit @@ -156,6 +237,12 @@ def test_group_failures_and_cap(registry, settings, trace, tmp_path): assert not result.ok and result.failure is FailureKind.ESCALATE assert "safety cap" in result.summary + settings.pr_debug_max_groups = 6 + result = asyncio.run(registry.get("pr.group_failures").handler( + _ctx(settings, trace, tmp_path, dict(state), params={"max_groups": 1}) + )) + assert not result.ok and "safety cap of 1" in result.summary + def test_debug_group_blocked_without_llm(registry, settings, trace, tmp_path): state = {"repo_path": "/tmp", "task_spec": {"pr": 7}} @@ -165,6 +252,26 @@ def test_debug_group_blocked_without_llm(registry, settings, trace, tmp_path): assert not result.ok and result.failure is FailureKind.BLOCKED +def test_debug_group_does_not_accept_model_success_without_a_commit( + registry, settings, trace, tmp_path, git_repo, monkeypatch, +): + from infermatrix_copilot.engine import agent_runtime + + async def claimed_success(*_args, **_kwargs): + return StepResult(True, summary="done"), { + "root_cause": "wrong condition", "fix_summary": "fixed", + "verification": "pytest passed", + } + + monkeypatch.setattr(agent_runtime, "run_agent_step", claimed_success) + state = {"repo_path": str(git_repo), "task_spec": {"pr": 7}} + ctx = _ctx(settings, trace, tmp_path, state) + ctx.item = {"signature": "one failing job", "jobs": ["ci"]} + result = asyncio.run(registry.get("agent.debug_group").handler(ctx)) + assert not result.ok and result.failure is FailureKind.BLOCKED + assert "observed commit" in result.summary + + def test_post_review_gating(registry, settings, trace, tmp_path): step = registry.get("pr.post_review") # post flag not set -> no-op diff --git a/test/test_profile_store.py b/test/test_profile_store.py index 6d8c37cc4..9d2606556 100644 --- a/test/test_profile_store.py +++ b/test/test_profile_store.py @@ -162,3 +162,24 @@ def test_malformed_ops_rejected_individually(tmp_path, bad): results = store.apply_ops([_fact_op(), bad], tier="consolidate") assert results[0] == "" and results[1] != "" assert "fmt" in store.facts # the good op still applied + + +def test_non_object_and_malformed_evidence_do_not_abort_later_operations(tmp_path): + store = ProfileStore(tmp_path / "profile") + results = store.apply_ops(["bad", _fact_op(evidence=42), _fact_op()]) + assert "must be an object" in results[0] + assert "list of strings" in results[1] + assert results[2] == "" + assert list(store.facts) == ["fmt"] + + +def test_unknown_tier_cannot_rewrite_profile(tmp_path): + store = ProfileStore(tmp_path / "profile") + assert store.apply_ops([_fact_op()]) == [""] + before = store.facts["fmt"].text + result = store.apply_ops( + [{"op": "rewrite_fact", "id": "fmt", "text": "replacement"}], + tier="typo", + ) + assert "unknown profile tier" in result[0] + assert store.facts["fmt"].text == before