From 55d6f323bcca4ff7d8eb355d87531f99bf4b1102 Mon Sep 17 00:00:00 2001 From: Ouroboros <311266734+ouroboros-agent@users.noreply.github.com> Date: Sun, 6 Sep 2026 09:30:00 +0000 Subject: [PATCH] fix(git): preserve local work and bind GitHub tools to Projects Verify existing local branches before unambiguous checkout. Route all GitHub issue and PR calls through the existing Project binding or explicit repository, retain native CLI credential discovery, and preserve Presence argument authority. Co-authored-by: Ouroboros <311266734+ouroboros-agent@users.noreply.github.com> --- docs/ARCHITECTURE.md | 4 +- docs/DEVELOPMENT.md | 6 + ouroboros/tools/git.py | 54 +++--- ouroboros/tools/github.py | 132 +++++++++---- ouroboros/tools/registry_core.py | 13 +- ouroboros/tools/registry_guards.py | 13 +- ouroboros/workspace_admission.py | 7 +- tests/test_github_project_target.py | 210 +++++++++++++++++++++ tests/test_repo_commit_branch_selection.py | 109 +++++++++++ tests/test_repo_commit_detached_head.py | 8 +- 10 files changed, 480 insertions(+), 76 deletions(-) create mode 100644 tests/test_github_project_target.py create mode 100644 tests/test_repo_commit_branch_selection.py diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 0326b44a2..524fb4cb0 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -400,7 +400,7 @@ server.py (Starlette+uvicorn) ← HTTP + WebSocket on configurable host:port (de │ ├── commit_gate.py ← Commit gate: `_record_commit_attempt` (LLM claim synthesis), `classify_review_block`/`attempt_block_class`, `check_identical_verdict_refusal`, `count_paid_review_cycles`/`check_review_cycles_ceiling`, `commit_review_contract_fingerprint` │ ├── git_rollback.py ← Wraps `git_ops.rollback_to_version` │ ├── git_pr.py ← Five PR tools (non-core) - │ ├── github.py ← Issue + PR tools (frozen tool module) + │ ├── github.py ← Issue + PR tools (frozen tool module): shared process binding selects the active Project; explicit repo flows through every subcall; missing implicit Project targets fail visibly. Discovery reads token sources or native CLI configuration without an authentication probe; explicit Hub/API transport retains its own target. │ ├── parallel_review.py ← Triad + scope review orchestration: assembly of both packets, the money admission call (`review_admission.py`), the scope-first hold, and the executor transitions that carry the admitting usage scope (`contextvars.copy_context`) into every seat │ ├── plan_review_references.py ← Reference projection that also writes its own provenance rows (`append_jsonl` into `logs/progress.jsonl`, `emit_log_event`), never a second plan authority │ ├── plan_review.py ← `plan_task` engine: evidence, packet, fan-out over the review substrate, `plan_review_state` v2, the shared `OUROBOROS_REVIEW_MAX_CYCLES` cap, free identical replays; no scouts, Atlas, or plan_class @@ -1185,6 +1185,8 @@ Disclosed delegated-isolation residuals (deliberate): a run whose owner's termin ### Git and commit review +Commit preparation verifies the exact local working-branch ref before an unambiguous checkout. A missing ref refuses without changing the current branch, index or files; remote guessing and implicit branch creation are disabled. Detached work retains the existing `checkout -B HEAD` recovery, and managed assisted merges retain transaction-owned precommit verification. + `tools/git.py` owns repository writes, staging, reviewed commit, rollback or restore, tags, push, and CI follow-up. File-edit tools validate their own atomic write shape; `mutation_attribution.py` captures the root-task baseline and projects only the clean-at-baseline system-repository delta — a changed pre-existing dirty path, stale or missing baseline, or failed scan blocks automatic staging. `commit_reviewed(paths=None)` stages only that attributed candidate, explicit paths must be a subset, and an empty candidate returns `GIT_NO_ATTRIBUTED_CHANGES`; managed update transactions keep their separate typed whole-tree authority. A reviewed commit is bound to one staged fingerprint. A cheap LLM-first advisory pass may run before the expensive gates; it is advisory, and skipping it never skips independently applicable tests, triad, applicable scope review, aggregation, or exact-SHA binding. The hermetic preflight runs the candidate in a disposable worktree and data root; triad and scope inspect the same staged snapshot, aggregation preserves actor evidence and obligations, and any mutation stales the binding. Managed exception: a managed-update resolution commit reviews the declared M0→S subject (`tools/review_subject.py`) and the commit gate binds S to the exact index write-tree the fingerprint pins. External review wrappers report readiness but do not grant commit authority. diff --git a/docs/DEVELOPMENT.md b/docs/DEVELOPMENT.md index 3119567a4..b3ffab8f2 100644 --- a/docs/DEVELOPMENT.md +++ b/docs/DEVELOPMENT.md @@ -72,6 +72,12 @@ rules have no automated surface — review-only. implicit sandbox. - Project-local installs may run within the workspace policy. Global/system installs remain safety-reviewed, and `sudo` is non-interactive (`sudo -n`). +- GitHub issue/PR tools resolve the same process binding as shell commands and + carry an explicit `repo` into every dependent call. Project focus overrides + ambient `GH_REPO`; broken room bindings refuse and file-less Projects need an + explicit repository. Presence may override its default repository only through + a host-selected argument binding. Native CLI configuration proves configuration, + not active authentication; discovery never logs in or probes the network. - Do not add a second scheduler for operator tooling or a generic CLI file manager. Use the task queue, attachments, logs, and artifact endpoints. diff --git a/ouroboros/tools/git.py b/ouroboros/tools/git.py index 8ce32cfd4..fc57d2008 100644 --- a/ouroboros/tools/git.py +++ b/ouroboros/tools/git.py @@ -1054,36 +1054,38 @@ def _prepare_review_commit_worktree( merges keep their live MERGE_HEAD and only run the existing transaction precommit verification. """ - is_detached = False + if managed_tx: + from supervisor.update_merge import managed_assisted_precommit_verify + + verified, error_message = managed_assisted_precommit_verify(managed_tx) + return False, "" if verified else error_message came_from_detached_checkout = False - if not managed_tx: - try: - current_branch = run_cmd( - ["git", "rev-parse", "--abbrev-ref", "HEAD"], cwd=ctx.repo_dir - ).strip() - is_detached = current_branch == "HEAD" - except Exception: - pass try: - if not managed_tx: - if is_detached: - run_cmd( - ["git", "checkout", "-B", ctx.branch_dev, "HEAD"], - cwd=ctx.repo_dir, - ) - came_from_detached_checkout = True - else: - run_cmd(["git", "checkout", ctx.branch_dev], cwd=ctx.repo_dir) + current_branch = run_cmd( + ["git", "rev-parse", "--abbrev-ref", "HEAD"], cwd=ctx.repo_dir + ).strip() + if current_branch != "HEAD": + # A missing branch must never resolve as a package path or remote guess. + run_cmd(["git", "show-ref", "--verify", f"refs/heads/{ctx.branch_dev}"], cwd=ctx.repo_dir) + except Exception as exc: + return False, ( + f"⚠️ GIT_BRANCH_UNAVAILABLE: cannot verify local branch {ctx.branch_dev!r}. " + f"Current branch, index and files were preserved. {_sanitize_git_error(str(exc))}" + ) + try: + if current_branch == "HEAD": + run_cmd(["git", "checkout", "-B", ctx.branch_dev, "HEAD"], cwd=ctx.repo_dir) + came_from_detached_checkout = True + else: + run_cmd(["git", "checkout", "--no-guess", ctx.branch_dev, "--"], cwd=ctx.repo_dir) except Exception as exc: error_message = _sanitize_git_error(str(exc)) - already_on_target = False try: - current_branch_after = run_cmd( + already_on_target = run_cmd( ["git", "rev-parse", "--abbrev-ref", "HEAD"], cwd=ctx.repo_dir - ).strip() - already_on_target = current_branch_after == ctx.branch_dev + ).strip() == ctx.branch_dev except Exception: - pass + already_on_target = False if not already_on_target: return came_from_detached_checkout, f"⚠️ GIT_ERROR (checkout): {error_message}" try: @@ -1103,12 +1105,6 @@ def _prepare_review_commit_worktree( "the checkout failure as an incidental dirty-tree no-op.\n" f"{unmerged}" ) - if managed_tx: - from supervisor.update_merge import managed_assisted_precommit_verify - - verified, error_message = managed_assisted_precommit_verify(managed_tx) - if not verified: - return came_from_detached_checkout, error_message return came_from_detached_checkout, "" diff --git a/ouroboros/tools/github.py b/ouroboros/tools/github.py index 01febc02a..fd3f0aa2d 100644 --- a/ouroboros/tools/github.py +++ b/ouroboros/tools/github.py @@ -5,6 +5,7 @@ from __future__ import annotations import json import logging import os +import pathlib import subprocess from typing import List, Optional @@ -33,17 +34,69 @@ def _gh_env(ctx: ToolContext) -> dict: return env -def _gh_cmd(args: List[str], ctx: ToolContext, timeout: int = 30, input_data: Optional[str] = None) -> str: - cmd = ["gh"] + args +def github_cli_configured() -> bool: + """Local credential configuration, not a live authentication assertion.""" + if github_token_from_env_or_settings(): + return True + config_dir = os.environ.get("GH_CONFIG_DIR", "") + if not config_dir: + base = os.environ.get("XDG_CONFIG_HOME", "") + config_dir = str(pathlib.Path(base) / "gh") if base else "" + if not config_dir: + from ouroboros.platform_layer import IS_WINDOWS + + app_data = os.environ.get("APPDATA", "") if IS_WINDOWS else "" + config_dir = str(pathlib.Path(app_data) / "GitHub CLI") if app_data else str(pathlib.Path.home() / ".config" / "gh") try: + import yaml + + hosts = yaml.safe_load((pathlib.Path(config_dir) / "hosts.yml").read_text(encoding="utf-8")) + return isinstance(hosts, dict) and any( + isinstance(host, dict) and bool(host.get("user") or host.get("users") or host.get("oauth_token")) + for host in hosts.values() + ) + except (OSError, ValueError, yaml.YAMLError): + return False + + +def _gh_cmd(args: List[str], ctx: ToolContext, timeout: int = 30, input_data: Optional[str] = None, + *, repo: Optional[str] = None) -> str: + # None keeps explicit API/Hub callers on their existing transport contract. + # The eight repository tools pass a string, including '' for Project focus. + try: + cwd, env = pathlib.Path(ctx.repo_dir), _gh_env(ctx) + cmd = ["gh", *args] + if repo is not None: + from ouroboros.tool_access import build_resolved_resource_binding + + metadata = getattr(ctx, "task_metadata", {}) + metadata = metadata if isinstance(metadata, dict) else {} + workspace = getattr(ctx, "workspace_root", None) + room_dir = str(metadata.get("_project_room_dir") or "") + project = str(getattr(ctx, "project_id", "") or "") + if not repo: + note = str(metadata.get("_project_room_note") or "") + selected = workspace or room_dir + if note or (selected and not pathlib.Path(selected).is_dir()): + return f"⚠️ GH_TARGET_UNAVAILABLE: {note or 'The selected Project directory is unavailable.'}" + if project and not selected: + return "⚠️ GH_TARGET_REQUIRED: this Project has no repository directory; pass repo='[HOST/]OWNER/REPO'." + binding = build_resolved_resource_binding(ctx, operation="shell", process_cwd="") + cwd = binding.target_path + if workspace and cwd != pathlib.Path(workspace).resolve(strict=False): + return "⚠️ GH_TARGET_UNAVAILABLE: the task's Project binding could not be resolved." + if workspace or room_dir or project: + env.pop("GH_REPO", None) # Ambient defaults cannot replace the selected Project. + if repo: + cmd.extend(["--repo", repo]) res = subprocess.run( cmd, - cwd=str(ctx.repo_dir), + cwd=str(cwd), capture_output=True, text=True, timeout=timeout, input=input_data, - env=_gh_env(ctx), + env=env, ) if res.returncode != 0: err = (res.stderr or "").strip() @@ -56,7 +109,7 @@ def _gh_cmd(args: List[str], ctx: ToolContext, timeout: int = 30, input_data: Op except Exception as e: return f"⚠️ GH_ERROR: {e}" -def _list_issues(ctx: ToolContext, state: str = "open", labels: str = "", limit: int = 20) -> str: +def _list_issues(ctx: ToolContext, state: str = "open", labels: str = "", limit: int = 20, repo: str = "") -> str: args = [ "issue", "list", "--state", state, @@ -66,7 +119,7 @@ def _list_issues(ctx: ToolContext, state: str = "open", labels: str = "", limit: if labels: args.extend(["--label", labels]) - raw = _gh_cmd(args, ctx) + raw = _gh_cmd(args, ctx, repo=repo) if raw.startswith("⚠️"): return raw @@ -94,7 +147,7 @@ def _list_issues(ctx: ToolContext, state: str = "open", labels: str = "", limit: return "\n".join(lines) -def _get_issue(ctx: ToolContext, number: int) -> str: +def _get_issue(ctx: ToolContext, number: int, repo: str = "") -> str: if number <= 0: return "⚠️ issue number must be positive" @@ -103,7 +156,7 @@ def _get_issue(ctx: ToolContext, number: int) -> str: "--json", "number,title,body,labels,createdAt,author,assignees,state,comments", ] - raw = _gh_cmd(args, ctx) + raw = _gh_cmd(args, ctx, repo=repo) if raw.startswith("⚠️"): return raw @@ -138,7 +191,7 @@ def _get_issue(ctx: ToolContext, number: int) -> str: return "\n".join(lines) -def _comment_on_issue(ctx: ToolContext, number: int, body: str) -> str: +def _comment_on_issue(ctx: ToolContext, number: int, body: str, repo: str = "") -> str: if number <= 0: return "⚠️ issue number must be positive" @@ -146,35 +199,35 @@ def _comment_on_issue(ctx: ToolContext, number: int, body: str) -> str: return "⚠️ Comment body cannot be empty." args = ["issue", "comment", str(number), "--body-file", "-"] - raw = _gh_cmd(args, ctx, input_data=body) + raw = _gh_cmd(args, ctx, input_data=body, repo=repo) if raw.startswith("⚠️"): return raw return f"✅ Comment added to issue #{number}." -def _close_issue(ctx: ToolContext, number: int, comment: str = "") -> str: +def _close_issue(ctx: ToolContext, number: int, comment: str = "", repo: str = "") -> str: if number <= 0: return "⚠️ issue number must be positive" if comment and comment.strip(): - result = _comment_on_issue(ctx, number, comment) + result = _comment_on_issue(ctx, number, comment, repo=repo) if result.startswith("⚠️"): return result args = ["issue", "close", str(number)] - raw = _gh_cmd(args, ctx) + raw = _gh_cmd(args, ctx, repo=repo) if raw.startswith("⚠️"): return raw return f"✅ Issue #{number} closed." -def _list_prs(ctx: ToolContext, state: str = "open", limit: int = 20) -> str: +def _list_prs(ctx: ToolContext, state: str = "open", limit: int = 20, repo: str = "") -> str: args = [ "pr", "list", "--state", state, "--limit", str(min(limit, 50)), "--json", "number,title,author,headRefName,baseRefName,createdAt,isDraft,reviewDecision,commits", ] - raw = _gh_cmd(args, ctx) + raw = _gh_cmd(args, ctx, repo=repo) if raw.startswith("⚠️"): return raw @@ -203,7 +256,7 @@ def _list_prs(ctx: ToolContext, state: str = "open", limit: int = 20) -> str: return "\n".join(lines) -def _get_pr(ctx: ToolContext, number: int) -> str: +def _get_pr(ctx: ToolContext, number: int, repo: str = "") -> str: if number <= 0: return "⚠️ PR number must be positive." @@ -213,7 +266,7 @@ def _get_pr(ctx: ToolContext, number: int) -> str: "createdAt,updatedAt,state,isDraft,reviewDecision,mergeable," "additions,deletions,changedFiles,commits,reviews,comments", ] - raw = _gh_cmd(meta_args, ctx, timeout=30) + raw = _gh_cmd(meta_args, ctx, timeout=30, repo=repo) if raw.startswith("⚠️"): return raw @@ -263,11 +316,9 @@ def _get_pr(ctx: ToolContext, number: int) -> str: author_str = "unknown" lines.append(f" {sha} | {author_str} | {msg}") shas_for_pick.append(full_sha) - lines.append( - f"\nSHAs for cherry_pick_pr_commits:\n {shas_for_pick}" - ) + lines.append(f"\nCommit SHAs:\n {shas_for_pick}") - diff_names_raw = _gh_cmd(["pr", "diff", str(number), "--name-only"], ctx, timeout=30) + diff_names_raw = _gh_cmd(["pr", "diff", str(number), "--name-only"], ctx, timeout=30, repo=repo) if not diff_names_raw.startswith("⚠️") and diff_names_raw.strip(): file_list = diff_names_raw.strip().splitlines() lines.append(f"\n**Changed files ({len(file_list)}):**") @@ -276,7 +327,7 @@ def _get_pr(ctx: ToolContext, number: int) -> str: if len(file_list) > 50: lines.append(f" ... and {len(file_list) - 50} more") - diff_raw = _gh_cmd(["pr", "diff", str(number)], ctx, timeout=60) + diff_raw = _gh_cmd(["pr", "diff", str(number)], ctx, timeout=60, repo=repo) if not diff_raw.startswith("⚠️") and diff_raw.strip(): lines.append("\n**Diff (truncated to 8000 chars):**\n```diff") lines.append(_truncate_with_notice(diff_raw, 8000)) @@ -296,42 +347,43 @@ def _get_pr(ctx: ToolContext, number: int) -> str: cm_body = _truncate_with_notice((cm.get("body") or "").strip(), 300) lines.append(f" @{cm_author}: {cm_body}") - lines.append( - f"\n**Integration steps:**\n" - f" 1. fetch_pr_ref(pr_number={number})\n" - f" 2. create_integration_branch(pr_number={number})\n" - f" 3. cherry_pick_pr_commits(shas=[...]) # SHAs above; use override_author only for placeholder identities\n" + if not (repo or getattr(ctx, "workspace_root", None) or getattr(ctx, "project_id", "")): + lines.append( + f"\n**Integration steps:**\n" + f" 1. fetch_pr_ref(pr_number={number})\n" + f" 2. create_integration_branch(pr_number={number})\n" + f" 3. cherry_pick_pr_commits(shas=[...]) # SHAs above; use override_author only for placeholder identities\n" f" 4. stage_adaptations() # optional; do NOT commit_reviewed on the integration branch\n" f" 5. stage_pr_merge(branch='integrate/pr-{number}') → preflight_review → commit_reviewed\n" - f" 6. comment_on_pr(number={number}, body='Integrated as ...')" - ) + f" 6. comment_on_pr(number={number}, body='Integrated as ...')" + ) return "\n".join(lines) -def _comment_on_pr(ctx: ToolContext, number: int, body: str) -> str: +def _comment_on_pr(ctx: ToolContext, number: int, body: str, repo: str = "") -> str: if number <= 0: return "⚠️ PR number must be positive." if not (body or "").strip(): return "⚠️ Comment body cannot be empty." args = ["pr", "comment", str(number), "--body-file", "-"] - raw = _gh_cmd(args, ctx, input_data=body) + raw = _gh_cmd(args, ctx, input_data=body, repo=repo) if raw.startswith("⚠️"): return raw return f"✅ Comment added to PR #{number}." -def _create_issue(ctx: ToolContext, title: str, body: str = "", labels: str = "") -> str: +def _create_issue(ctx: ToolContext, title: str, body: str = "", labels: str = "", repo: str = "") -> str: if not title or not title.strip(): return "⚠️ Issue title cannot be empty." args = ["issue", "create", f"--title={title}"] if body: args.append("--body-file=-") - raw = _gh_cmd(args, ctx, input_data=body) + raw = _gh_cmd(args, ctx, input_data=body, repo=repo) else: - raw = _gh_cmd(args, ctx) + raw = _gh_cmd(args, ctx, repo=repo) if labels: if not raw.startswith("⚠️"): @@ -340,14 +392,14 @@ def _create_issue(ctx: ToolContext, title: str, body: str = "", labels: str = "" if match: issue_num = int(match.group(1)) label_args = ["issue", "edit", str(issue_num), f"--add-label={labels}"] - _gh_cmd(label_args, ctx) + _gh_cmd(label_args, ctx, repo=repo) if raw.startswith("⚠️"): return raw return f"✅ Issue created: {raw}" def get_tools() -> List[ToolEntry]: - return [ + tools = [ ToolEntry("list_github_prs", { "name": "list_github_prs", "description": ( @@ -370,7 +422,7 @@ def get_tools() -> List[ToolEntry]: "Get full details of a GitHub PR: metadata, description, commit list " "with original author names/emails, changed files list, diff/patch " "(truncated to 8000 chars), review comments, and mergeable state. " - "Shows the exact SHAs needed for cherry_pick_pr_commits." + "Includes exact commit SHAs for the selected repository." ), "parameters": {"type": "object", "properties": { "number": {"type": "integer", "description": "PR number"}, @@ -436,3 +488,9 @@ def get_tools() -> List[ToolEntry]: }, "required": ["title"]}, }, _create_issue), ] + for entry in tools: + entry.schema["parameters"]["properties"]["repo"] = { + "type": "string", "default": "", + "description": "Explicit [HOST/]OWNER/REPO. Omit for the active Project repository; required for a Project without a repository folder. An omitted HOST follows GitHub CLI host configuration.", + } + return tools diff --git a/ouroboros/tools/registry_core.py b/ouroboros/tools/registry_core.py index 1a08a5a5e..8f080323b 100644 --- a/ouroboros/tools/registry_core.py +++ b/ouroboros/tools/registry_core.py @@ -170,8 +170,19 @@ def _presence_binding_allowed(ctx: Any, binding: Any) -> bool: def _presence_bound_args(ctx: Any, name: str, args: Any) -> tuple[dict[str, Any], str]: try: - from ouroboros.presence_authority import apply_presence_argument_bindings + from ouroboros.presence_authority import apply_presence_argument_bindings, presence_ceiling_from_context + ceiling = presence_ceiling_from_context(ctx) + if ceiling is not None and dict(args or {}).get("repo"): + from ouroboros.tools.github import get_tools as github_tools + + if name in {entry.name for entry in github_tools()}: + grant = next((item for item in ceiling.tool_grants if item.name == name), None) + if grant is None or not any(binding.argument_path == ("repo",) for binding in grant.bindings): + return {}, ( + "⚠️ PRESENCE_ARGUMENT_BINDING_BLOCKED: an explicit GitHub repository " + "requires the presence's host-selected repo argument binding." + ) bound = apply_presence_argument_bindings(ctx, name, dict(args or {})) if not _presence_tool_allowed(ctx, name): return {}, ( diff --git a/ouroboros/tools/registry_guards.py b/ouroboros/tools/registry_guards.py index ca3c2425a..ebbfb8fd9 100644 --- a/ouroboros/tools/registry_guards.py +++ b/ouroboros/tools/registry_guards.py @@ -547,8 +547,17 @@ def _builtin_tool_availability(name: str, ctx: Any = None) -> tuple[bool, str, s return True, "", "" except Exception: return True, "", "" - if tool in _GITHUB_TOKEN_TOOLS and not os.environ.get("GITHUB_TOKEN", "").strip(): - return False, "missing_credential", "GITHUB_TOKEN" + if tool in _GITHUB_TOKEN_TOOLS: + detail = "GITHUB_TOKEN" + if tool in {"run_ci_tests", "submit_skill_to_hub", "generate_evolution_stats"}: + configured = bool(os.environ.get("GITHUB_TOKEN", "").strip()) + else: + from ouroboros.tools.github import github_cli_configured + + configured = github_cli_configured() + detail = "GITHUB_TOKEN or configured GitHub CLI" + if not configured: + return False, "missing_credential", detail return True, "", "" diff --git a/ouroboros/workspace_admission.py b/ouroboros/workspace_admission.py index 883272936..ac15e8b3e 100644 --- a/ouroboros/workspace_admission.py +++ b/ouroboros/workspace_admission.py @@ -186,8 +186,11 @@ def room_chat_lens_dir(drive_root: Any, project_id: str) -> tuple[str, str]: project = get_project(drive_root, pid) or {} raw = str(project.get("working_dir") or "").strip() - except Exception: - return "", "" + except Exception as exc: + return "", ( + f"project {pid!r} registry entry is unreadable ({type(exc).__name__}: {exc}) — " + "cannot determine the room's repository" + ) if not raw: return "", "" try: diff --git a/tests/test_github_project_target.py b/tests/test_github_project_target.py new file mode 100644 index 000000000..7a5d1c56e --- /dev/null +++ b/tests/test_github_project_target.py @@ -0,0 +1,210 @@ +"""GitHub tools retain Project selection and explicit targets through every subcall.""" + +import json +import subprocess +from types import SimpleNamespace + +import pytest + +from ouroboros.tools import github +from ouroboros.tools.registry import ToolContext + + +def _context(tmp_path, kind): + system = tmp_path / "system" + project = tmp_path / "project" + system.mkdir(exist_ok=True) + project.mkdir(exist_ok=True) + ctx = ToolContext(repo_dir=system, drive_root=tmp_path / "data", task_id="task-fixture") + if kind == "queued": + ctx.workspace_root, ctx.workspace_mode, ctx.project_id = project, "external", "project-fixture" + elif kind == "room": + ctx.is_direct_chat, ctx.project_id = True, "project-fixture" + ctx.task_metadata = {"_project_room_dir": str(project)} + return ctx, project if kind != "system" else system + + +@pytest.fixture +def gh_calls(monkeypatch): + calls = [] + monkeypatch.setattr(github, "github_token_from_env_or_settings", lambda: "fixture-token") + + def run(argv, **kwargs): + calls.append((argv, kwargs)) + output = "" + if argv[1:3] in (["issue", "list"], ["pr", "list"]): + output = "[]" + elif argv[1:3] in (["issue", "view"], ["pr", "view"]): + output = json.dumps({"number": 7, "title": "Fixture", "state": "OPEN", "author": {"login": "fixture"}}) + elif argv[1:3] == ["issue", "create"]: + output = "https://github.com/owner/selected/issues/7" + return SimpleNamespace(returncode=0, stdout=output, stderr="") + + monkeypatch.setattr(subprocess, "run", run) + return calls + + +_CALLS = [ + ("list_github_issues", {}, 1), ("get_github_issue", {"number": 7}, 1), + ("comment_on_issue", {"number": 7, "body": "text"}, 1), + ("close_github_issue", {"number": 7, "comment": "closing"}, 2), + ("create_github_issue", {"title": "Title", "body": "Body", "labels": "bug"}, 2), + ("list_github_prs", {}, 1), ("get_github_pr", {"number": 7}, 3), + ("comment_on_pr", {"number": 7, "body": "text"}, 1), +] + + +@pytest.mark.parametrize("kind", ["system", "queued", "room"]) +@pytest.mark.parametrize("name,args,count", _CALLS) +@pytest.mark.parametrize("repo", ["", "github.example/owner/selected"]) +def test_every_repository_tool_keeps_target_in_all_subcalls(tmp_path, gh_calls, monkeypatch, kind, name, args, count, repo): + ctx, expected_cwd = _context(tmp_path, kind) + monkeypatch.setenv("GH_REPO", "unrelated/wrong-repo") + monkeypatch.setenv("GH_HOST", "configured.example") + entry = next(item for item in github.get_tools() if item.name == name) + + result = entry.handler(ctx, **args, repo=repo) + + assert not result.startswith("⚠️"), result + assert len(gh_calls) == count + assert "repo" in entry.schema["parameters"]["properties"] + for argv, kwargs in gh_calls: + assert kwargs["cwd"] == str(expected_cwd) + if repo: + assert argv[-2:] == ["--repo", repo] + else: + assert "--repo" not in argv + if kind != "system": + assert "GH_REPO" not in kwargs["env"] + assert kwargs["env"]["GH_HOST"] == "configured.example" + if name == "get_github_pr" and (kind != "system" or repo): + assert "fetch_pr_ref(" not in result + assert "stage_pr_merge(" not in result + + +@pytest.mark.parametrize("failure", ["note", "missing-room", "fileless", "missing-workspace", "invalid-workspace-mode"]) +def test_unusable_project_never_calls_gh_on_system_repo(tmp_path, gh_calls, failure): + ctx, project = _context(tmp_path, "queued" if "workspace" in failure else "room") + if failure == "note": + ctx.task_metadata = {"_project_room_note": "registry unavailable"} + elif failure == "fileless": + ctx.task_metadata = {} + elif failure == "invalid-workspace-mode": + ctx.workspace_mode = "" + else: + project.rmdir() + + result = github._list_issues(ctx) + + assert "GH_TARGET_" in result + assert gh_calls == [] + + +def test_fileless_room_accepts_explicit_repo(tmp_path, gh_calls): + ctx, _ = _context(tmp_path, "room") + ctx.task_metadata = {} + assert not github._get_issue(ctx, 7, repo="owner/selected").startswith("⚠️") + assert gh_calls[0][0][-2:] == ["--repo", "owner/selected"] + + +def test_generic_hub_transport_keeps_explicit_api_contract(tmp_path, gh_calls, monkeypatch): + ctx, _ = _context(tmp_path, "room") + ctx.task_metadata = {"_project_room_note": "registry unavailable"} + monkeypatch.setenv("GH_REPO", "configured/hub") + github._gh_cmd(["api", "/repos/owner/hub/contents/catalog.json"], ctx) + assert gh_calls[0][0] == ["gh", "api", "/repos/owner/hub/contents/catalog.json"] + assert gh_calls[0][1]["cwd"] == str(ctx.repo_dir) + assert gh_calls[0][1]["env"]["GH_REPO"] == "configured/hub" + + +def test_room_registry_failure_is_visible(tmp_path, monkeypatch): + from ouroboros import projects_registry + from ouroboros.workspace_admission import room_chat_lens_dir + + def fail(*args): + raise OSError("fixture registry failure") + + monkeypatch.setattr(projects_registry, "get_project", fail) + directory, note = room_chat_lens_dir(tmp_path, "project-fixture") + assert directory == "" + assert "registry entry is unreadable" in note + assert "OSError" in note + + +@pytest.mark.parametrize("token", ["GITHUB_TOKEN", "GH_TOKEN", "settings"]) +def test_cli_discovery_accepts_the_execution_token_sources(tmp_path, monkeypatch, token): + from ouroboros import config + from ouroboros.tools.registry_guards import _builtin_tool_availability + + monkeypatch.delenv("GITHUB_TOKEN", raising=False) + monkeypatch.delenv("GH_TOKEN", raising=False) + monkeypatch.setattr(config, "load_settings", lambda: {"GITHUB_TOKEN": "fixture"} if token == "settings" else {}) + if token != "settings": + monkeypatch.setenv(token, "fixture") + ctx, _ = _context(tmp_path, "queued") + assert _builtin_tool_availability("get_github_issue", ctx)[0] is True + + +def test_cli_store_metadata_enables_only_cli_tools_without_probing(tmp_path, monkeypatch): + from ouroboros.tools.registry_guards import _builtin_tool_availability + + monkeypatch.setattr(github, "github_token_from_env_or_settings", lambda: "") + monkeypatch.delenv("GITHUB_TOKEN", raising=False) + monkeypatch.setenv("GH_CONFIG_DIR", str(tmp_path / "gh")) + (tmp_path / "gh").mkdir() + (tmp_path / "gh" / "hosts.yml").write_text("github.com:\n user: fixture\n", encoding="utf-8") + + def no_probe(*args, **kwargs): + raise AssertionError("discovery must not spawn an authentication probe") + + monkeypatch.setattr(subprocess, "run", no_probe) + ctx, _ = _context(tmp_path, "queued") + for name, _, _ in _CALLS: + assert _builtin_tool_availability(name, ctx)[0] is True + for name in ("run_ci_tests", "submit_skill_to_hub", "generate_evolution_stats"): + assert _builtin_tool_availability(name, ctx) == (False, "missing_credential", "GITHUB_TOKEN") + + +@pytest.mark.parametrize("bound,explicit", [(False, False), (False, True), (True, True)]) +def test_presence_repository_selection_keeps_host_argument_authority(tmp_path, gh_calls, bound, explicit): + from ouroboros.presence_authority import PresenceCapabilityCeiling, PresenceToolGrant, presence_ceiling_payload + from ouroboros.presence_capabilities import PresenceArgumentBinding + from ouroboros.tools.registry import ToolRegistry + + bindings = (PresenceArgumentBinding(("repo",), "static", static_value="owner/allowed"),) if bound else () + ceiling = PresenceCapabilityCeiling( + skill_name="fixture", skill_content_hash="a" * 64, profile_fingerprint="b" * 64, + state_fingerprint="c" * 64, selection_fingerprint="d" * 64, model_slot="main", + inline_max_rounds=10, tool_grants=(PresenceToolGrant("get_github_issue", bindings),), + resource_grants=(), digest="e" * 64, + ) + ctx, _ = _context(tmp_path, "system") + ctx.task_contract = {"capability_ceiling": presence_ceiling_payload(ceiling)} + registry = ToolRegistry(repo_dir=ctx.repo_dir, drive_root=ctx.drive_root) + registry.set_context(ctx) + args = {"number": 7, **({"repo": "owner/unselected"} if explicit else {})} + + result = registry.execute("get_github_issue", args) + + if explicit and not bound: + assert "PRESENCE_ARGUMENT_BINDING_BLOCKED" in result + assert not gh_calls + else: + assert "Issue #7" in result + assert len(gh_calls) == 1 + if bound: + assert gh_calls[0][0][-2:] == ["--repo", "owner/allowed"] + else: + assert "--repo" not in gh_calls[0][0] + + +def test_child_does_not_gain_github_access_from_explicit_repo(tmp_path, gh_calls): + from ouroboros.tools.registry import ToolRegistry + + ctx, _ = _context(tmp_path, "queued") + ctx.task_metadata = {"delegation_role": "subagent"} + registry = ToolRegistry(repo_dir=ctx.repo_dir, drive_root=ctx.drive_root) + registry.set_context(ctx) + result = registry.execute("get_github_issue", {"number": 7, "repo": "owner/unselected"}) + assert "BLOCKED" in result + assert not gh_calls diff --git a/tests/test_repo_commit_branch_selection.py b/tests/test_repo_commit_branch_selection.py new file mode 100644 index 000000000..f662f7f57 --- /dev/null +++ b/tests/test_repo_commit_branch_selection.py @@ -0,0 +1,109 @@ +"""Commit preparation preserves dirty package files when the local branch is missing.""" + +import subprocess +from types import SimpleNamespace + +import pytest + +from ouroboros.tools import git as git_tools + +pytestmark = pytest.mark.serial + + +def _git(repo, *args): + result = subprocess.run( + ["git", *args], cwd=repo, capture_output=True, text=True, check=True, + ) + return result.stdout.strip() + + +def _repository(tmp_path, remote_count): + repo = tmp_path / "repo" + repo.mkdir() + _git(repo, "init", "-b", "feature") + package = repo / "ouroboros" + package.mkdir() + tracked = package / "module.py" + tracked.write_text("base = 1\n", encoding="utf-8") + _git(repo, "add", "ouroboros/module.py") + _git(repo, "-c", "user.name=Fixture", "-c", "user.email=fixture@example.invalid", + "commit", "-m", "fixture base") + for remote in ("origin", "managed")[:remote_count]: + _git(repo, "remote", "add", remote, f"https://example.invalid/{remote}/repo.git") + _git(repo, "update-ref", f"refs/remotes/{remote}/ouroboros", "HEAD") + tracked.write_text("staged = 2\n", encoding="utf-8") + _git(repo, "add", "ouroboros/module.py") + tracked.write_text("unstaged = 3\n", encoding="utf-8") + (package / "new.txt").write_text("new work\n", encoding="utf-8") + (repo / "outside.txt").write_text("outside work\n", encoding="utf-8") + return repo + + +def _snapshot(repo): + # Exact index and file bytes distinguish staged work from unstaged work. + return { + "branch": _git(repo, "symbolic-ref", "HEAD"), + "head": _git(repo, "rev-parse", "HEAD"), + "refs": _git(repo, "show-ref"), + "index": (repo / ".git" / "index").read_bytes(), + "files": {str(path.relative_to(repo)): path.read_bytes() + for path in repo.rglob("*") if path.is_file() and ".git" not in path.parts}, + } + + +@pytest.mark.parametrize("remote_count", [0, 1, 2]) +def test_missing_local_branch_preserves_candidate_without_remote_guess(tmp_path, remote_count): + repo = _repository(tmp_path, remote_count) + before = _snapshot(repo) + + detached, error = git_tools._prepare_review_commit_worktree( + SimpleNamespace(repo_dir=repo, branch_dev="ouroboros"), None, + ) + + assert detached is False + assert "GIT_BRANCH_UNAVAILABLE" in error + assert "ouroboros" in error + assert _snapshot(repo) == before + + +@pytest.mark.parametrize("remote_count", [0, 1, 2]) +def test_existing_local_branch_switch_preserves_staged_and_unstaged_work(tmp_path, remote_count): + repo = _repository(tmp_path, remote_count) + _git(repo, "branch", "ouroboros") + before = _snapshot(repo) + staged = _git(repo, "show", ":ouroboros/module.py") + + detached, error = git_tools._prepare_review_commit_worktree( + SimpleNamespace(repo_dir=repo, branch_dev="ouroboros"), None, + ) + + assert (detached, error) == (False, "") + after = _snapshot(repo) + assert after["branch"] == "refs/heads/ouroboros" + assert after["head"] == before["head"] + assert after["refs"] == before["refs"] + assert after["files"] == before["files"] + assert _git(repo, "show", ":ouroboros/module.py") == staged + + +@pytest.mark.parametrize("verified", [True, False]) +def test_managed_preparation_uses_transaction_verification_only(tmp_path, monkeypatch, verified): + from supervisor import update_merge + + transaction = {"id": "fixture-managed-merge"} + seen = [] + + def verify(value): + seen.append(value) + return verified, "managed refusal" + + def no_checkout(*args, **kwargs): + raise AssertionError("managed preparation must not select another branch") + + monkeypatch.setattr(update_merge, "managed_assisted_precommit_verify", verify) + monkeypatch.setattr(git_tools, "run_cmd", no_checkout) + result = git_tools._prepare_review_commit_worktree( + SimpleNamespace(repo_dir=tmp_path, branch_dev="ouroboros"), transaction, + ) + assert seen == [transaction] + assert result == (False, "" if verified else "managed refusal") diff --git a/tests/test_repo_commit_detached_head.py b/tests/test_repo_commit_detached_head.py index c97416139..d37938c14 100644 --- a/tests/test_repo_commit_detached_head.py +++ b/tests/test_repo_commit_detached_head.py @@ -3,7 +3,7 @@ fix_verification.sh checks the pure-git LOGIC; this exercises the real ouroboros.tools.git code: on a detached HEAD it must issue `git checkout -B ctx.branch_dev HEAD` (preserve the in-flight commit) and NEVER the plain `git checkout ctx.branch_dev` that orphans it, and flow -came_from_detached_checkout=True into the stage cycle; on a normal branch the path is unchanged. +came_from_detached_checkout=True into the stage cycle; a normal branch uses a verified local ref. """ from __future__ import annotations @@ -72,10 +72,10 @@ def test_detached_head_force_moves_with_B_and_never_orphans(tmp_path): "the detached-reconcile flag must flow into the stage cycle (for the GIT_LOST diagnostic)" -def test_on_branch_uses_plain_checkout_no_force_move(tmp_path): +def test_on_branch_uses_unambiguous_checkout_no_force_move(tmp_path): checkout_cmds, stage_kwargs = _drive_commit(tmp_path, head_branch="ouroboros") # on-branch - assert "git checkout ouroboros" in checkout_cmds, \ - f"on-branch must use the plain checkout (byte-identical to pre-fix); got {checkout_cmds}" + assert "git checkout --no-guess ouroboros --" in checkout_cmds, \ + f"on-branch must select the local branch without path or remote guessing; got {checkout_cmds}" assert not any("-B" in c for c in checkout_cmds), \ f"on-branch must NOT force-move; got {checkout_cmds}" assert not stage_kwargs.get("came_from_detached_checkout"), \