diff --git a/doc/architecture/SPEC/adapters/base.md b/doc/architecture/SPEC/adapters/base.md index 89a8b1013..98d7c80eb 100644 --- a/doc/architecture/SPEC/adapters/base.md +++ b/doc/architecture/SPEC/adapters/base.md @@ -1,6 +1,6 @@ # adapters/base.py —— 规范 - + `LOC ~423 · 边缘（仓库知识） · refactor-status: ok` @@ -28,6 +28,9 @@ 获胜且**绝不被修改**；仍未解析的变量 → 返回 ""（fail-closed，能力缺口路径）。 - **未知 adapter 名抛 `AdapterNotFound`**（子类）；把"不存在"当兼容路径的调用方 只能捕获这个子类 —— 已知 adapter 的畸形 manifest 必须仍是硬失败。 +- `load_adapter` 将 manifest 读取、编码与 YAML 解析错误归一为 `AdapterError`， + 并要求 manifest 为映射、`name` 为文本、`repo` 为映射；调用方不能把这些错误 + 当作 adapter 缺失而忽略。 - `capabilities` 由 manifest 推导（repo.path/language.*/ci.provider/upstream.*/ modules）+ 显式的 `capabilities:` —— 与 playbook 的 `requires:` 匹配。 - `high_risk_modules` = 标了 `risk: high` 的模块（喂给 patch-review，**A5**）。 @@ -52,5 +55,6 @@ ## 重构备注 两个关注点在这里共存得很干净：`RepoAdapter`（访问器）+ 注册表 + 引导。 如果引导继续长大（更丰富的指纹），可考虑 `adapters/bootstrap.py`。 -**把 `capabilities` 推导保持为 planner 信任的唯一来源** —— 不要在 `cli.resolve` 里 -复制一份能力逻辑（它目前只为 REPO_PATHS **补上** repo.path，这是可以接受的）。 +**把 `capabilities` 推导保持为 planner 信任的唯一来源** —— +`app.repository_context` 只为实际 checkout **补上** `repo.path`， +并将同一 adapter policy 交给执行层。 diff --git a/doc/architecture/SPEC/app/core.md b/doc/architecture/SPEC/app/core.md index 3b29433e8..f9533fab5 100644 --- a/doc/architecture/SPEC/app/core.md +++ b/doc/architecture/SPEC/app/core.md @@ -8,6 +8,13 @@ executes or resumes a playbook, and reserves/executes durable runs. The CLI imports it through `cli.copilot` for compatibility; the application never imports CLI or MCP transports. +Repository checkout, capabilities, and adapter policy come from +`app.repository_context.RepositoryContextResolver`. Planning and execution +use the same context contract. A missing adapter retains the legacy +unknown-capabilities path when a checkout exists; a broken known adapter +blocks the run. Execution rechecks the context after planning and refuses a +changed checkout or policy before starting the executor. + `Copilot` keeps reservation and run-path methods as compatibility delegates to `app.reservation.RunReservation`. It owns workflow resolution and child execution; it does not own idempotency indexing or run ID validation. diff --git a/doc/architecture/SPEC/app/repository_context.md b/doc/architecture/SPEC/app/repository_context.md new file mode 100644 index 000000000..c68b5eac3 --- /dev/null +++ b/doc/architecture/SPEC/app/repository_context.md @@ -0,0 +1,20 @@ +# app/repository_context.py — repository context boundary + + + +`RepositoryContextResolver.for_spec` produces the checkout, planner +capabilities, protected branches, and high-risk modules for one `TaskSpec`. +The frozen `spec.repo_path` wins over ambient `REPO_PATHS` and the adapter +path. The resulting immutable `RepositoryContext` is passed from planning to +execution and re-resolved before execution starts; a change blocks the run. + +Only genuine adapter absence permits compatibility behavior. With no adapter, +capabilities are unknown (`None`) when a checkout exists and empty otherwise. +A known adapter with a missing, unreadable, malformed, or mismatched manifest +raises `AdapterError`. Malformed checkout or policy fields also raise +`AdapterError`; no execution path silently substitutes the default policy. + +The resolver owns no planner, executor, persistence, or transport behavior. +`Copilot` retains its older path and adapter helpers as delegates. Tests in +`test/test_repository_context.py` cover checkout precedence, policy +propagation, malformed manifests, and context drift. diff --git a/src/infermatrix_copilot/adapters/base.py b/src/infermatrix_copilot/adapters/base.py index b74620e7f..ffb97dd96 100644 --- a/src/infermatrix_copilot/adapters/base.py +++ b/src/infermatrix_copilot/adapters/base.py @@ -295,10 +295,17 @@ def load_adapter(adapter_dir: str | Path) -> RepoAdapter: manifest_path = root / "manifest.yaml" if not manifest_path.exists(): raise AdapterError(f"no manifest.yaml in {root}") - manifest = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) or {} + try: + manifest = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) or {} + except (OSError, UnicodeError, yaml.YAMLError) as exc: + raise AdapterError(f"cannot load adapter manifest in {root}: {exc}") from exc + if not isinstance(manifest, dict): + raise AdapterError(f"adapter {root.name}: manifest must be a mapping") for section in REQUIRED_SECTIONS: if section not in manifest: raise AdapterError(f"adapter {root.name}: missing required section '{section}'") + if not isinstance(manifest["name"], str) or not isinstance(manifest["repo"], dict): + raise AdapterError(f"adapter {root.name}: name must be text and repo a mapping") return RepoAdapter(name=manifest["name"], root=root, manifest=manifest) diff --git a/src/infermatrix_copilot/app/core.py b/src/infermatrix_copilot/app/core.py index 65aeb59af..de3001dcc 100644 --- a/src/infermatrix_copilot/app/core.py +++ b/src/infermatrix_copilot/app/core.py @@ -17,6 +17,7 @@ from typing import NamedTuple from .. import run_status as rs +from ..adapters.base import AdapterError from ..config import Settings, TierNotConfiguredError from ..engine.executor import Executor from ..engine.lifecycle import RunLock, RunLockHeld, run_guarded @@ -34,6 +35,7 @@ from ..ui import style from ..metrics import format_metrics_line from .reservation import RunReservation +from .repository_context import RepositoryContext, RepositoryContextResolver class GateOutcome(NamedTuple): @@ -70,6 +72,7 @@ def __init__(self, settings: Settings | None = None): and is filled by the first execution.""" self.settings = settings or Settings() self.reservations = RunReservation(self.settings) + self.repository_context = RepositoryContextResolver(self.settings) # provider-registry seam: a real llm.LLM under backend "api" # (byte-identical), a HarnessLLM adapter under a harness backend from ..providers import llm_for @@ -89,41 +92,15 @@ def resolve(self, spec: TaskSpec) -> Resolution: capability set so the planner only reuses playbooks the target supports. Capabilities come from the repo's adapter (if any), plus `repo.path` when a path is resolvable even without a adapter (REPO_PATHS works adapter-less).""" - # Only genuine adapter ABSENCE takes the unknown-capabilities - # compatibility path — a malformed/unreadable KNOWN adapter must fail - # closed here, not fail open into capabilities=None and recall a - # playbook whose requirements were never established. - from ..adapters.base import AdapterError, AdapterNotFound, AdapterRegistry - adapter_name = spec.repo.replace("-", "_") - try: - adapter = AdapterRegistry(self.settings.adapters_dir).resolve( - name=adapter_name) - except AdapterNotFound: - # the registry resolves by DECLARED manifest name and skips - # manifest-less directories — so "not found" alone does not - # prove absence. Only a genuinely absent directory is the - # v1-compatible path; an existing directory that failed to - # load/resolve (deleted manifest, wrong name:) fails closed. - if (Path(self.settings.adapters_dir) / adapter_name).exists(): - raise AdapterError( - f"adapter directory {adapter_name!r} exists but did not " - "load/resolve (missing manifest.yaml or mismatched " - "name:) — refusing to plan with unknown capabilities") - adapter = None # absence: v1-compatible - except FileNotFoundError: - adapter = None # no adapters directory at all - if adapter is None: - # No adapter means capabilities are UNKNOWN, not zero — the - # store's requires-filter (now covering exact-repo playbooks too) - # skips on None, keeping adapter-less setups v1-compatible - # instead of silently dropping every playbook with a `requires:`. - if self._repo_path_for(spec): - return self.planner.resolve(spec, capabilities=None) - return self.planner.resolve(spec, capabilities=set()) - capabilities = set(adapter.capabilities) - if self._repo_path_for(spec): # REPO_PATHS works adapter-less - capabilities.add("repo.path") - return self.planner.resolve(spec, capabilities=capabilities) + return self._resolve_with_context(spec)[0] + + def _resolve_with_context( + self, spec: TaskSpec, + ) -> tuple[Resolution, RepositoryContext]: + context = self.repository_context.for_spec(spec) + return self.planner.resolve( + spec, capabilities=context.capabilities, + ), context def _plan_review_gate(self, resolution: Resolution, spec: TaskSpec, assume_yes: bool) -> bool: @@ -162,8 +139,8 @@ def run_task(self, spec: TaskSpec, *, assume_yes: bool = False, `plan_only` prints the resolved plan and returns 0 without running; `assume_yes` skips the interactive confirm.""" try: - resolution = self.resolve(spec) - except PlanningError as exc: + resolution, repo_context = self._resolve_with_context(spec) + except (PlanningError, AdapterError) as exc: print(style("✋ cannot plan: ", "red", "bold") + str(exc)) return BLOCKED_EXIT # tier preflight (plan v2): backend availability is DEPLOYMENT state, @@ -215,7 +192,8 @@ def run_task(self, spec: TaskSpec, *, assume_yes: bool = False, "invocation_id": os.environ.get("IMX_INVOCATION_ID", ""), }, indent=2)) return self._execute(resolution.playbook, spec, run_dir, - resolution_mode=resolution.mode, tier=resolution.tier) + resolution_mode=resolution.mode, tier=resolution.tier, + planned_context=repo_context) def run_playbook(self, name: str, *, params: dict | None = None, report_only: bool = False, assume_yes: bool = False, @@ -287,7 +265,8 @@ def run_playbook(self, name: str, *, params: dict | None = None, def _execute(self, playbook, spec: TaskSpec, run_dir: Path, *, resolution_mode: str = "resume", tier: str = "?", - resuming: bool = False, held_lock: RunLock | None = None) -> int: + resuming: bool = False, held_lock: RunLock | None = None, + planned_context: RepositoryContext | None = None) -> int: """Run a resolved `playbook` to completion in `run_dir`: init tracing + notifier, seed the shared state (repo path, push policy, protected branches / high-risk modules from the adapter when present), drive the @@ -298,6 +277,16 @@ def _execute(self, playbook, spec: TaskSpec, run_dir: Path, *, the code but owns the terminal `run_status.json` write.""" self.last_run_dir = run_dir self.last_blocked_reason = "" + try: + repo_context = self.repository_context.for_spec(spec) + except AdapterError as exc: + self.last_blocked_reason = f"repository context invalid: {exc}" + print(style("✋ ", "red", "bold") + self.last_blocked_reason) + return BLOCKED_EXIT + if planned_context is not None and repo_context != planned_context: + self.last_blocked_reason = "repository context changed after planning" + print(style("✋ ", "red", "bold") + self.last_blocked_reason) + return BLOCKED_EXIT lock = held_lock if lock is None: try: @@ -349,17 +338,13 @@ def _execute(self, playbook, spec: TaskSpec, run_dir: Path, *, playbook=playbook.name, tier=tier) state: dict = { "task_spec": spec.model_dump(), - "repo_path": self._repo_path_for(spec), + "repo_path": repo_context.repo_path, "push_policy": PushPolicy(), # steps may replace with a derived policy - "protected_branches": self.settings.protected_branches, + "protected_branches": list(repo_context.protected_branches), "resuming": resuming, } - adapter = self._adapter_for(spec.repo) - if adapter is not None: - # repo knowledge from the adapter, not core settings (v2 P0 fix #5) - state["protected_branches"] = adapter.protected_branches - if adapter.high_risk_modules: - state["high_risk_modules"] = adapter.high_risk_modules + if repo_context.high_risk_modules: + state["high_risk_modules"] = list(repo_context.high_risk_modules) executor = Executor(self.registry, self.settings, run_dir=run_dir, trace=trace, llm=self.llm, notifier=notifier) # run_guarded finalizes inside the event loop: playbooks that @@ -509,8 +494,8 @@ def _execute_reserved_locked(self, run_dir: Path, lock: RunLock, rs.mark(run_dir, rs.FAILED, note=f"policy/request rejected: {exc}") return 1 try: - resolution = self.resolve(spec) - except PlanningError as exc: + resolution, repo_context = self._resolve_with_context(spec) + except (PlanningError, AdapterError) as exc: rs.mark(run_dir, rs.BLOCKED, note=f"cannot plan: {exc}") return BLOCKED_EXIT try: # tier preflight is re-enforced in the MCP child (authoritative) @@ -526,7 +511,7 @@ def _execute_reserved_locked(self, run_dir: Path, lock: RunLock, try: code = self._execute(resolution.playbook, spec, run_dir, resolution_mode=resolution.mode, tier=resolution.tier, - held_lock=lock) + held_lock=lock, planned_context=repo_context) except Exception as exc: # a crash still leaves a terminal record rs.mark(run_dir, rs.FAILED, note=f"{type(exc).__name__}: {exc}") raise @@ -540,14 +525,8 @@ def _execute_reserved_locked(self, run_dir: Path, lock: RunLock, return code def _adapter_for(self, repo: str): - """The repo's registered adapter, or None (never raises).""" - try: - from ..adapters.base import AdapterRegistry - - return AdapterRegistry(self.settings.adapters_dir).resolve( - name=repo.replace("-", "_")) - except Exception: - return None + """Compatibility lookup; a broken known adapter remains an error.""" + return self.repository_context.adapter_for(repo) def _repo_path_for(self, spec: TaskSpec) -> str: """The checkout THIS spec runs against: its frozen `repo_path` first, @@ -560,7 +539,7 @@ def _repo_path_for(self, spec: TaskSpec) -> str: resolve a playbook that requires it — while a perfectly valid checkout sat in the spec. With both configured and differing, planning would evaluate one checkout and execution would run against another.""" - return spec.repo_path or self._resolve_repo_path(spec.repo) + return self.repository_context.repo_path_for(spec) def _resolve_repo_path(self, repo: str) -> str: """REPO_PATHS first; fall back to the repo's adapter manifest (adapter zero @@ -568,13 +547,7 @@ def _resolve_repo_path(self, repo: str) -> str: Alias-only: callers that hold a TaskSpec want `_repo_path_for`, which honors a frozen per-run path.""" - p = self.settings.repo_path(repo) - if p: - return str(p) - adapter = self._adapter_for(repo) - if adapter and adapter.repo_path: - return adapter.repo_path - return "" + return self.repository_context.ambient_repo_path(repo) # -- built-ins --------------------------------------------------------------- def status(self) -> str: diff --git a/src/infermatrix_copilot/app/repository_context.py b/src/infermatrix_copilot/app/repository_context.py new file mode 100644 index 000000000..957fe70b8 --- /dev/null +++ b/src/infermatrix_copilot/app/repository_context.py @@ -0,0 +1,110 @@ +"""One repository context for planning capabilities and execution policy.""" + +from __future__ import annotations + +from dataclasses import dataclass +from pathlib import Path + +from ..adapters.base import AdapterError, AdapterNotFound, AdapterRegistry, RepoAdapter +from ..config import Settings +from ..task_spec import TaskSpec + + +@dataclass(frozen=True) +class RepositoryContext: + repo_path: str + capabilities: frozenset[str] | None + protected_branches: tuple[str, ...] + high_risk_modules: tuple[str, ...] + + +class RepositoryContextResolver: + """Resolve one run's checkout and adapter policy without hiding bad adapters.""" + + def __init__(self, settings: Settings) -> None: + self.settings = settings + + def adapter_for(self, repo: str) -> RepoAdapter | None: + name = repo.replace("-", "_") + try: + return AdapterRegistry(self.settings.adapters_dir).resolve(name=name) + except AdapterNotFound: + # A directory with a missing or mismatched manifest is a broken + # known adapter, not the compatibility path for an unknown repo. + if (Path(self.settings.adapters_dir) / name).exists(): + raise AdapterError( + f"adapter directory {name!r} exists but did not " + "load/resolve (missing manifest.yaml or mismatched " + "name:) — refusing to use unknown repository policy" + ) + return None + except (OSError, UnicodeError) as exc: + raise AdapterError( + f"cannot inspect repository adapters for {repo!r}: {exc}" + ) from exc + + def ambient_repo_path(self, repo: str) -> str: + return self.for_spec(TaskSpec(kind="repo_rebase", repo=repo)).repo_path + + def repo_path_for(self, spec: TaskSpec) -> str: + return self.for_spec(spec).repo_path + + def for_spec(self, spec: TaskSpec) -> RepositoryContext: + adapter = self.adapter_for(spec.repo) + configured = self.settings.repo_path(spec.repo) + repo_path = spec.repo_path or (str(configured) if configured else "") + if adapter is None: + # No adapter means capabilities are unknown if a checkout exists; + # the planner's v1 compatibility behavior deliberately uses None. + capabilities = None if repo_path else frozenset() + protected = tuple(self.settings.protected_branches) + high_risk = () + else: + try: + manifest = adapter.manifest + raw_path = manifest["repo"].get("path", "") + raw_capabilities = manifest.get("capabilities") + raw_modules = manifest.get("modules") + raw_push = manifest.get("push") + if "path" in manifest["repo"] and not isinstance(raw_path, str): + raise TypeError("repo.path must be text") + if raw_capabilities is None: + raw_capabilities = [] + if (not isinstance(raw_capabilities, list) + or not all(isinstance(item, str) for item in raw_capabilities)): + raise TypeError("capabilities must be a list of text") + if raw_modules is None: + raw_modules = {} + if not isinstance(raw_modules, dict): + raise TypeError("modules must be a mapping") + if raw_push is None: + raw_push = {} + if not isinstance(raw_push, dict): + raise TypeError("push must be a mapping") + raw_branches = raw_push.get("protected_branches", ["main"]) + if (not isinstance(raw_branches, list) + or not all(isinstance(item, str) for item in raw_branches)): + raise TypeError("push.protected_branches must be a list of text") + if not repo_path: + repo_path = adapter.repo_path + if not isinstance(repo_path, str): + raise TypeError("repo.path must be text") + known = set(adapter.capabilities) + protected = tuple(adapter.protected_branches) + high_risk = tuple(adapter.high_risk_modules) + if not all(isinstance(item, str) for item in (*known, *protected, *high_risk)): + raise TypeError("capabilities and policy entries must be text") + except (AttributeError, TypeError, ValueError) as exc: + raise AdapterError( + f"adapter {adapter.name!r} has invalid repository policy: {exc}" + ) from exc + known.discard("repo.path") + if repo_path: + known.add("repo.path") + capabilities = frozenset(known) + return RepositoryContext( + repo_path=repo_path, + capabilities=capabilities, + protected_branches=protected, + high_risk_modules=high_risk, + ) diff --git a/test/test_repository_context.py b/test/test_repository_context.py new file mode 100644 index 000000000..e0a1cc7cb --- /dev/null +++ b/test/test_repository_context.py @@ -0,0 +1,96 @@ +from pathlib import Path + +import pytest + +from infermatrix_copilot.app.core import Copilot +from infermatrix_copilot.app.repository_context import RepositoryContextResolver +from infermatrix_copilot.notify import BLOCKED_EXIT +from infermatrix_copilot.task_spec import TaskSpec + + +def _adapter(settings, checkout: Path, *, branches: str = "[main]") -> Path: + root = settings.adapters_dir / "vllm_omni" + root.mkdir(parents=True, exist_ok=True) + manifest = root / "manifest.yaml" + manifest.write_text( + "name: vllm_omni\nstatus: active\nrepo:\n" + f" path: {checkout}\n" + "modules:\n scheduler:\n risk: high\n" + f"push:\n protected_branches: {branches}\n" + ) + return manifest + + +def test_planning_and_execution_context_share_checkout_and_adapter_policy( + settings, tmp_path, +): + ambient = tmp_path / "ambient" + frozen = tmp_path / "frozen" + _adapter(settings, ambient, branches="[main, release]") + spec = TaskSpec(kind="repo_rebase", repo="vllm-omni", repo_path=str(frozen)) + + context = RepositoryContextResolver(settings).for_spec(spec) + + assert context.repo_path == str(frozen) + assert "repo.path" in context.capabilities + assert context.protected_branches == ("main", "release") + assert context.high_risk_modules == ("scheduler",) + + +@pytest.mark.parametrize("manifest", ( + "{not yaml: [", + "name: vllm_omni\nrepo: []\n", + "name: vllm_omni\nrepo: {}\npush: []\n", + "name: vllm_omni\nrepo:\n path: 123\n", + "name: vllm_omni\nrepo: {}\ncapabilities: reviewer\n", + "name: vllm_omni\nrepo: {}\nmodules: []\n", + "name: vllm_omni\nrepo: {}\npush:\n protected_branches: main\n", +)) +def test_known_broken_adapter_blocks_execution_before_run_artifacts( + settings, tmp_path, manifest, +): + bad = settings.adapters_dir / "vllm_omni" + bad.mkdir(parents=True) + (bad / "manifest.yaml").write_text(manifest) + copilot = Copilot(settings) + run_dir = tmp_path / "run-bad-adapter" + + code = copilot._execute(None, TaskSpec(kind="repo_rebase"), run_dir) + + assert code == BLOCKED_EXIT + assert "repository context invalid" in copilot.last_blocked_reason + assert not run_dir.exists() + assert copilot.run_task(TaskSpec(kind="repo_rebase")) == BLOCKED_EXIT + + +def test_adapter_policy_change_after_planning_blocks_execution( + settings, tmp_path, +): + manifest = _adapter(settings, tmp_path / "checkout") + spec = TaskSpec(kind="repo_rebase", repo="vllm-omni") + copilot = Copilot(settings) + planned = copilot.repository_context.for_spec(spec) + manifest.write_text(manifest.read_text().replace("[main]", "[main, release]")) + run_dir = tmp_path / "run-changed-policy" + + code = copilot._execute(None, spec, run_dir, planned_context=planned) + + assert code == BLOCKED_EXIT + assert copilot.last_blocked_reason == "repository context changed after planning" + assert not run_dir.exists() + + +def test_unresolved_adapter_path_does_not_claim_checkout_capability( + settings, tmp_path, monkeypatch, +): + monkeypatch.delenv("IMX_TEST_MISSING_REPOSITORY_PATH", raising=False) + _adapter(settings, tmp_path / "unused") + manifest = settings.adapters_dir / "vllm_omni" / "manifest.yaml" + manifest.write_text(manifest.read_text().replace( + str(tmp_path / "unused"), "${IMX_TEST_MISSING_REPOSITORY_PATH}")) + + context = RepositoryContextResolver(settings).for_spec( + TaskSpec(kind="repo_rebase", repo="vllm-omni")) + + assert context.repo_path == "" + assert "repo.path" not in context.capabilities