From 7d79e7e161b62c1ae04867d216ded6cb4170d33f Mon Sep 17 00:00:00 2001 From: Jonny Luca <320529100+JonnyLuca@users.noreply.github.com> Date: Sun, 6 Sep 2026 19:40:35 +0000 Subject: [PATCH 1/8] Configure explicit GitHub execution accounts and document script ownership. --- AGENTS.md | 15 ++ CONTRIBUTING.md | 16 +- DESIGN.md | 113 +++++++- README.md | 22 +- docs/github-accounts.md | 77 ++++++ src/agent_cli/github_accounts.py | 184 +++++++++++++ src/agent_cli/github_act.py | 29 +- src/agent_cli/main.py | 8 +- src/agent_cli/skills/error-fix/SKILL.md | 10 + src/agent_cli/skills/pr-review/SKILL.md | 10 + src/agent_cli/skills/review-loop/SKILL.md | 10 + src/agent_cli/skills/session-store/SKILL.md | 11 + src/agent_cli/skills/spine/SKILL.md | 10 + src/agent_cli/supervise.py | 4 + src/agent_cli/watch.py | 28 +- tests/github_support.py | 44 +++ tests/test_github_accounts.py | 281 ++++++++++++++++++++ tests/test_github_act.py | 11 +- tests/test_run.py | 3 + tests/test_supervise.py | 11 +- tests/test_watch.py | 23 +- 21 files changed, 886 insertions(+), 34 deletions(-) create mode 100644 docs/github-accounts.md create mode 100644 src/agent_cli/github_accounts.py create mode 100644 tests/github_support.py create mode 100644 tests/test_github_accounts.py diff --git a/AGENTS.md b/AGENTS.md index c961b08..2ff976a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -5,6 +5,21 @@ Read [CONTRIBUTING.md](CONTRIBUTING.md), [DESIGN.md](DESIGN.md), and changing this repository. Skill contracts live next to the client: `agent skills path` (spine, review-loop, pr-review, error-fix). +Static scripts own assignment acceptance, its issue confirmation before the +implementation lane starts, every lane/subagent start, test execution, and all +GitHub communication. Implementers and reviewers never run tests, spawn agents, +or access GitHub themselves. See DESIGN.md §§19.1 and 19.7. Distinguish required +behavior from implemented and verified behavior; never invent evidence. + +Models never start monitors, poll status, or wait for CI or other events. Return +results or blockers to the script when there is no more work. The script owns +monitoring and informs a model when an observed event provides useful work. + +Installation defaults for GitHub accounts, AI accounts, roles, and selections +are unconfigured (`NULL`). Add them explicitly through configuration, with no +fixed count. See DESIGN.md §19.8 and docs/github-accounts.md for the implemented +GitHub configuration and remaining AI/role configuration gap. + Draft publication is immediate after the first signed task commit; see the lifecycle. A draft plus local tests is not done. Ready for review is signed commits on a branch in this repository, grok quality and logic then Codex diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 7d2754c..29a2520 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -7,11 +7,25 @@ - Public repository: English for commits and comments. The visible pull-request summary is an `EN:` block, optionally followed by a labeled `DE:` block. - Do not name private repositories, internal hostnames, or internal infrastructure. - Add or update tests in the same change. -- Run `pytest` on the exact clean signed final head before Ready for review. Tests need PostgreSQL (`AGENT_TEST_PG` or a local `initdb`). Full pytest is not a gate for the first draft publication. +- The static script runs `pytest` on the exact clean signed final head before Ready for review. Model lanes never execute tests themselves. Tests need PostgreSQL (`AGENT_TEST_PG` or a local `initdb`). Full pytest is not a gate for the first draft publication. - Pytest (or any green local suite) is a **check**, not Ready for review and not completion. ## Ready for review +The static script starts all implementation and review lanes, including each +implementation pass that addresses findings. Model lanes never launch subagents +or other lanes and never interact with GitHub. All GitHub reads and writes are +script operations. See [DESIGN.md §§19.1–19.7](DESIGN.md#191-responsibility-split) +for the responsibility split and the required assignment workflow. + +CI waiting and all other monitoring belong to the script. A model must not +start a monitor or poll for progress; it returns its result or blocker when its +work is exhausted. The script detects events and informs a lane when useful. + +New integrations must preserve the unconfigured (`NULL`) installation default +for accounts, roles, and selections. Configuration is explicit; there is no +fallback identity or fixed account/role count. See DESIGN.md §19.8. + A draft plus local tests is not done. Do not claim the pull request is finished, done, or completed at that point — including after leave-draft. Draft timing and CI ownership while the draft is open are defined in [docs/pull-request-lifecycle.md](docs/pull-request-lifecycle.md). Ready for review requires all of: diff --git a/DESIGN.md b/DESIGN.md index c4f257b..57ad7e5 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -37,6 +37,7 @@ The AI session talks **only** to the local database. Scripts perform every actio | Store engine | Local PostgreSQL on loopback / Unix socket under `$AGENT_HOME`. Not world-reachable. | | Catalog | Generic `activity` rows (`type` + payload). Session tags are the types present. | | Skills | Optional, requested. `spine`, `review-loop`, `pr-review`, and `error-fix` exist as skills. They are **not** on by default. | +| Initial configuration | GitHub accounts, AI accounts, roles, and their selections start unconfigured (`NULL`). Add them explicitly through configuration; no fixed account or role count. See §19.8. | | Runtime | This public client. Team-specific rules live elsewhere and must not ship a second store binary. | | Session mail | Addressed to a **session id**. Delivery does not require a subscription. | | TUI knock | Script wakes the session with only `da ist Post id `. The agent reads that row from local Postgres. | @@ -342,7 +343,7 @@ v1 types (mechanism only): The agent never learns a merge from a human prompt and never calls GitHub to ask “is it merged?”. -When this device has a `pr.open` row whose script result includes the PR number/url, a **script** watches that PR. On merge it inserts `pr.merged` on the **same session** (`payload`: repo, number, url, merge SHA, merged_at). That insert `NOTIFY`s `agent_inbox` (and enqueues `wake` if needed) with the new activity id. The device daemon’s knock child and §10 state machine emit `da ist Post id `. The watcher does not `tmux send-keys` itself. The agent `SELECT`s the row and decides what to do. +When this device has a `pr.open` row whose script result includes the PR number/url, a **script** watches that PR. On merge it inserts `pr.merged` on the **same session** (`payload`: repo, number, url, merge SHA, merged_at). That insert `NOTIFY`s `agent_inbox` (and enqueues `wake` if needed) with the new activity id. The device daemon’s knock child and §10 state machine emit `da ist Post id `. The watcher does not `tmux send-keys` itself. The agent `SELECT`s the row and supplies any analysis or implementation result; the script owns workflow progression (§19.1). The watcher runs on this device (write owner). It is a script, not the model. The model’s next turn is the knock plus the row — not a `gh` command. @@ -350,11 +351,14 @@ The watcher runs on this device (write owner). It is a script, not the model. Th ### Outside facts (example: issue assigned) -The script reads GitHub; the model does not. +The script reads GitHub; the model does not. The following describes the existing +assignment watcher. The required end-to-end workflow and its script-only +responsibilities are in [§19.7](#197-issue-assignment-to-human-merge); implementation +boundaries are recorded there separately. Allowlist file `$AGENT_HOME/watch.json` key `assigned_repos` (non-empty list of `Owner/repo` strings). Missing or empty is an error; there is no default list. -The first successful scan records `assigned_watch_since` and the assigned `session_id` and dispatches nothing. Later scans consider assignments whose latest matching `assigned` event is at or after that cursor, skipping `assigned_at` values already stored. Changing `session_id` after that pin is an error. The scan uses this device’s paired GitHub login; a missing pair or a `gh api user` mismatch is an error. The queue head is the already-knocked inflight item if any, then remaining items oldest first. +The first successful scan records `assigned_watch_since` and the assigned `session_id` and dispatches nothing. Later scans consider assignments whose latest matching `assigned` event is at or after that cursor, skipping `assigned_at` values already stored. Changing `session_id` after that pin is an error. The scan uses the session's explicitly configured GitHub execution account; a missing account or a `gh api user` mismatch is an error. This account is independent of hub pairing. The queue head is the already-knocked inflight item if any, then remaining items oldest first. The writer is this device. All assignments share **one** runner session (`watch.json` `session_id`, default `assigned`, characters `A-Za-z0-9_-` only). That auto-created session attaches `spine`, `review-loop`, and `pr-review`; an existing row under the same id must already be `kind=runner`. Other sessions still attach skills themselves. There is one tmux/Grok terminal, not one per issue. Working files go to `$AGENT_HOME/sessions/` or `$AGENT_SESSION_ROOT/`. New `issue.assigned` rows enqueue on that session (`payload`: repo, number, url, title, body, assigned_at, assignee, mandate). The insert does not notify the knock daemon. The script pushes own events, writes `MANDATE.md` / `QUEUE.md` (no issue body), starts Grok only if that session is not already attached, then knocks at most the head of the queue (`da ist Post id `). A knock of `issue.assigned` rewrites those files immediately before send. Further knocks stay queued until the **supervise script** records `issue.assigned.ack` with `payload.assigned_id`. The model must not insert that ack. The scan watermark `assigned_watch_since` is the scan clock, not the last seen GitHub event time — that is the no-backfill rule. @@ -435,7 +439,7 @@ agent cli-bridge [--port 7846] Local dashboard binds `127.0.0.1` only. -The AI is not expected to type hub HTTP, `gh`, or himalaya. It inserts `activity` (and `query.request` / `subscription.set` / `mail.reply` / `mail.seen`). Scripts watch the store. `agent github pending` is the GitHub executor for owned pending `pr.open`, `comment.post`, `review.post`, and `issue.write` rows. `agent mail pending` is the mailbox executor for owned pending `mail.reply` and `mail.seen` rows. The knock poll (device daemon knock child) also runs `scan_github` and `scan_mail`. `agent run` git-pushes (no force) when `pushed` is open and measures GitHub mergeability and checks when `mergeable` is open. +The AI must not call hub HTTP, `gh`, or himalaya; scripts perform these operations. It inserts `activity` (and `query.request` / `subscription.set` / `mail.reply` / `mail.seen`). Scripts watch the store. `agent github pending` is the GitHub executor for owned pending `pr.open`, `comment.post`, `review.post`, and `issue.write` rows. `agent mail pending` is the mailbox executor for owned pending `mail.reply` and `mail.seen` rows. The knock poll (device daemon knock child) also runs `scan_github` and `scan_mail`. `agent run` git-pushes (no force) when `pushed` is open and measures GitHub mergeability and checks when `mergeable` is open. ## 17. Control @@ -503,6 +507,10 @@ The rules below were already implied by §§1–17. They are now explicit so a l |---|---|---| | Semantic diagnosis, hypothesis, patch text, review findings, PR title/body draft | AI | `activity` intent (`investigate.step`, …); implementer/reviewer rows when those skills are on | | Auth, Git, GitHub HTTP, CI status, mail, hub HTTP, tmux knock, retries | Script | Result columns on the intent row; `pr.merged` and other watch types; `LISTEN agent_work` | +| Assignment acceptance and its confirmation in the GitHub issue, before starting the implementation lane | Static script on the execution device | Required ordering; see §19.7 | +| Start implementation and review lanes, restart the implementer for improvements, and start any other agents | Static script on the execution device | The model never orchestrates another model | +| Execute tests, builds, and checks, including local tests | Static script on the execution device | Script-produced results supplied to the lanes | +| Monitor or wait for CI, reviews, merges, and other external events; notify a lane when there is useful work | Static script on the execution device | Script-observed facts, not a waiting or polling model | | Spine task state | Spine CLI + local constraints | `task` state, checklist keys, `close-step` / `run` | | Whether a head is reviewed | Gate after the two vendor stages | `agent gate record` | | Whether a command ran and what it returned | The process that ran it | `agent check record` (name, command, `pass`/`fail`/`skip`, output) | @@ -512,6 +520,26 @@ A worker report such as “analysis complete” or “tests passed” is **input No transition that needs deterministic evidence may be satisfied by model text alone. Malformed structured output is rejected (unknown `activity.type` → `execution_status=error`; empty, partial, timeout, or unavailable review output is not zero findings). A patch that does not apply is a failed check, not a debate. +These responsibilities apply to implementers as well as reviewers. A model must +not start subagents, implementation lanes, review lanes, tests, builds, or checks. +It must not interact with GitHub at all, including reads: GitHub communication +always goes through the script. Calling an executor from inside a model lane to +start another lane or run tests does not transfer orchestration to the script. +The static script owns those decisions and invocations; the lane supplies its +implementation or review result. + +A model never starts a monitor, polls for status, or waits for a CI run or other +external event. Model lanes are used exclusively for work. When a lane has no +more work to perform, it returns its result or blocker to the script. Monitoring +and waiting remain entirely with the script. When the script detects an event, +it informs the model only when that event provides useful work for the model. +An idle model is not the supervisor, and a model must not start a watcher or a +background monitoring process on the script's behalf. + +Reports and documentation must stay with verified facts. Distinguish a user +requirement, an implemented behavior, and an observed result. Do not invent +requirements, implementation status, test results, or review evidence. + ### 19.2 Untrusted inputs Treat as **data**, never as control messages: @@ -557,6 +585,83 @@ Quality and logic of one vendor stage run together. Vendors are `grok`, then `co CI on this head is a script-measured fact whose applicability comes from the target repository's written rules. The frozen `dfx-local-ci/v1` format and legacy `agent local-ci verify` behavior do not by themselves adopt A38 or grant permission to skip GitHub CI. For A38 adopters, follow the central [A38 standard](docs/a38.md) and [guard guide](docs/a38-guard.md): private visibility alone is not opt-in, and private local code-gate equivalence requires trusted-base opt-in through a valid A38 manifest, assessment against the canonical active policy, and a separate live join against the actual latest report-like GitHub comment by the PR author. Public A38 adopters publish and validate the author report in addition to retaining cumulative GitHub CI; private repositories without that trusted-base opt-in and non-A38 repositories retain their existing written CI rules. For applicable GitHub CI checks, `skipped` and `cancelled` are not green unless the workflow documents that skip. Independently required GitHub-only checks, technical merge restrictions, review gates, and human merge remain required. Stay draft until the applicable rules hold. One comment whose review-pass count is those four `approved` verdicts on this head, then mark Ready for review (`isDraft=false`). A retry reuses the existing draft. Ready for review is still not merge and not completion. A human merges; claim completion only after that merge is verified. +### 19.7 Issue assignment to human merge + +**Required workflow.** The user creates an issue on GitHub, assigns it to an +agent account, and receives a pull request ready for human review and merge on +GitHub. The static script runs on the configured execution device. Deployment +hostnames belong in the deployment configuration, not this public repository. + +1. The script detects and accepts the assignment and confirms acceptance in the + GitHub issue. This acceptance and confirmation are script work, never model + work, and happen before the script starts the implementation lane. +2. The script starts the implementation lane with the assigned work. The model + implements the change and returns its result to the script. +3. The script starts the review lane. When improvements are needed, the script + starts the implementer again with the findings. The script owns every lane + start in the implementation/review loop; no model starts another model. +4. The script executes the required tests and checks and supplies the results to + the lanes. The implementer may change tests as part of the implementation but + may not execute them. Local test execution and any additional agents are + exclusively script responsibilities. +5. The script performs all GitHub communication, including issue and PR reads, + comments, PR publication, and status updates. A lane may provide content for + the script, but never communicates with GitHub itself. User-facing questions + and blockers, when needed, are communicated through GitHub by the script; + GitHub replies return through the script. The user need not operate a terminal + or a separate dashboard to manage the assigned issue. + When the lane has no further work, it returns to the script; it does not + monitor GitHub or wait for CI. The script observes subsequent events and + informs a lane when further work is useful. +6. Publication and readiness follow the existing + [pull-request lifecycle](docs/pull-request-lifecycle.md): the script publishes + a draft immediately after the first signed task commit, while tests and + reviews may still be pending. The script marks Ready for review only after + the required checks and reviews hold on the final head. These steps do not + wait for this numbered list to finish before publishing the first draft. +7. The human reviews and merges on GitHub. Completion requires verification of + that human merge, as defined in the lifecycle. + +**Existing implementation boundaries.** This requirement is not a claim that +the complete workflow is already implemented or enabled on a deployment: + +- [`watch.py`](src/agent_cli/watch.py) implements assignment scanning, a queue, + workspace files, and session dispatch. `dispatch_assigned` does not publish an + acceptance comment before starting the session. +- [`daemon.py`](src/agent_cli/daemon.py) starts knock, dashboard, CLI bridge, and + paired sync. It does not start `agent watch assigned --follow`. +- The shipped `ask=False` path in + [`supervise.py`](src/agent_cli/supervise.py) does not acknowledge completion of + an assignment from verified PR results. Its optional closed-question path is + not that verification. +- [`github_act.py`](src/agent_cli/github_act.py) provides the GitHub activity + executor. Its existence alone does not establish that model lanes cannot + access GitHub or execute tests or other agents. + +This section defines the required responsibility boundary. It does not claim +that a sandbox or other technical enforcement has been implemented. + +### 19.8 Configuration starts empty + +**Required default: `NULL`.** Installing Agent must not preconfigure or select +GitHub accounts, AI accounts, roles, or account/role assignments. Operators add +accounts and define roles explicitly through configuration, with no hard-coded +limit on their number. A missing setting is unconfigured, not permission to +select a built-in identity, provider account, or role. + +The device-local [GitHub account configuration](docs/github-accounts.md) implements +explicit GitHub accounts and session bindings. An absent or empty configuration +does not authorize GitHub execution. The executor verifies the selected login +and never falls back to an ambient login or a different configured account. +GitHub execution identities do not change the device's hub identity or transfer +ownership of store rows. + +**Remaining implementation boundary:** this GitHub configuration does not yet +implement configurable AI accounts or user-defined roles. The role/vendor lists +and model choices in `lane.py`, and the Grok default in `runtime.py`, still +contain fixed values. They must not be presented as satisfying the complete +empty-default configuration requirement. + ## 20. Refused: hub as a coding control plane An external architecture draft proposed turning the hub into an authoritative Error-to-PR control plane: hub-owned Task objects, leases, a worker scheduler, production-error ingestion as the first workflow, and `READY_FOR_PR` then deterministic pull-request creation. diff --git a/README.md b/README.md index b7c4077..bfb3e8d 100644 --- a/README.md +++ b/README.md @@ -8,6 +8,26 @@ This device is the write owner of its own rows. The local store is PostgreSQL on The [A38 standard](docs/a38.md) defines repository-owned local test requirements and author reports using the existing local-CI format. `agent a38` measures and validates reports; the [dfx pr guard](docs/a38-guard.md) explains repository rules and checks author comments without executing pull-request code. +The required GitHub issue-to-PR workflow is defined in +[DESIGN.md §19.7](DESIGN.md#197-issue-assignment-to-human-merge), together with the +current implementation boundaries. A static script accepts the assignment and +confirms it in the issue before starting the implementation lane. Scripts own +all lane starts, tests, and GitHub communication; model lanes do not start +subagents, run tests, or access GitHub. The user works in GitHub and merges the +reviewed PR there. This is a workflow requirement, not a claim of a complete +deployed integration. + +Model lanes perform work and return results or blockers. They never start +monitors, poll statuses, or wait for CI. Scripts own monitoring and inform a +model when an observed event provides useful work; see DESIGN.md §19.1. + +Accounts and roles must start unconfigured (`NULL`); see +[DESIGN.md §19.8](DESIGN.md#198-configuration-starts-empty). Configure GitHub +execution accounts and session bindings explicitly in +[`github-accounts.json`](docs/github-accounts.md). There is no default GitHub +account or fallback to the host login. AI-account and role configuration remain +separate implementation work, as recorded in the design. + ## Install ```bash @@ -115,7 +135,7 @@ The error-fix executor find-or-creates the implement task and isolated worktree; { "assigned_repos": ["Owner/repo"], "session_id": "assigned" } ``` -Missing or empty `assigned_repos` is an error. `session_id` is optional, defaults to `assigned`, and may contain only `A-Za-z0-9_-`. A session already present under that id must be `kind=runner`. The auto-created runner session attaches `spine`, `review-loop`, and `pr-review` (those skills stay opt-in for every other session). The working directory is `$AGENT_HOME/sessions/` unless `AGENT_SESSION_ROOT` is set. The first successful scan records the `assigned_watch_since` watermark and the assigned session id, and creates no activities. Changing `session_id` after that pin is an error. The scan uses the paired GitHub login; a missing pair or a `gh api user` mismatch is an error. Later scans enqueue `issue.assigned` on **that one** runner session, push to the hub, write `MANDATE.md` / `QUEUE.md`, and start Grok only if that session is not already attached. The insert does not notify the knock daemon. There is one terminal; further assignments wait in the knock queue until the supervise script records `issue.assigned.ack` with `payload.assigned_id` set to that activity id. The follow CLI does not ack from pane text. `MANDATE.md` lists session and activity ids. `QUEUE.md` lists ids and urls. Neither file contains issue bodies. Use `--follow` for a 30s loop, or cron for one-shot runs. +Missing or empty `assigned_repos` is an error. `session_id` is optional, defaults to `assigned`, and may contain only `A-Za-z0-9_-`. A session already present under that id must be `kind=runner`. The auto-created runner session attaches `spine`, `review-loop`, and `pr-review` (those skills stay opt-in for every other session). The working directory is `$AGENT_HOME/sessions/` unless `AGENT_SESSION_ROOT` is set. The first successful scan records the `assigned_watch_since` watermark and the assigned session id, and creates no activities. Changing `session_id` after that pin is an error. The scan uses the session account from `github-accounts.json`; a missing binding or a `gh api user` mismatch is an error. Hub pairing is separate and still used for sync. Later scans enqueue `issue.assigned` on **that one** runner session, push to the hub, write `MANDATE.md` / `QUEUE.md`, and start Grok only if that session is not already attached. The insert does not notify the knock daemon. There is one terminal; further assignments wait in the knock queue until the supervise script records `issue.assigned.ack` with `payload.assigned_id` set to that activity id. The follow CLI does not ack from pane text. `MANDATE.md` lists session and activity ids. `QUEUE.md` lists ids and urls. Neither file contains issue bodies. Use `--follow` for a 30s loop, or cron for one-shot runs. ### Session terminal control diff --git a/docs/github-accounts.md b/docs/github-accounts.md new file mode 100644 index 0000000..3a47e23 --- /dev/null +++ b/docs/github-accounts.md @@ -0,0 +1,77 @@ +# GitHub execution accounts + +Static scripts select GitHub accounts from `$AGENT_HOME/github-accounts.json`. +Installation creates no accounts or bindings. A missing file, `{}`, or null/empty +`accounts` and `sessions` leaves GitHub execution unconfigured. There is no +implicit account from hub pairing, the host login, or environment tokens. + +The following is an operator-supplied example, not an installed default: + +```json +{ + "accounts": { + "worker-a": { + "login": "example-worker-a", + "gh_config_dir": "/absolute/path/to/worker-a/gh", + "git": { + "name": "Example Worker A", + "email": "worker-a@example.com", + "signing_format": "ssh", + "signing_key": "/absolute/path/to/worker-a/signing-key" + } + }, + "worker-b": { + "login": "example-worker-b", + "gh_config_dir": "/absolute/path/to/worker-b/gh" + } + }, + "sessions": { + "implementation-session": "worker-a", + "review-session": "worker-b" + } +} +``` + +Add as many named accounts and session bindings as needed; no fixed account list +or count is built in. Each account references its own GitHub CLI configuration +directory. Tokens belong there, not in this manifest, activity payloads, or git. +The optional `git` object supplies the identity and signing configuration for +scripted Git operations; it is required when that account performs a Git push. +Signing formats are Git's `ssh`, `openpgp`, or `x509` values. GitHub operations +without Git writes need only `login` and `gh_config_dir`. + +For an account whose credentials and signing key live in a container, an optional +`command_prefix` supplies the static executor argv, for example +`["docker", "exec", "-i", "worker-container"]`. An optional `worktree_paths` +object maps absolute host worktree roots to absolute paths in that executor, +for example `{"/srv/worker/data": "/data"}`. The longest matching root is used +for Git's `-C` argument. Account configuration and signing-key paths refer to +the executor's filesystem. These are trusted operator settings, not commands +from a model or an issue. No prefix or path mapping is installed by default. + +The executor runs `gh api user` with that configuration before execution and +requires the returned login to match (case-insensitive). It clears inherited +GitHub token variables for the child process, does not change the process-wide +environment, and does not switch the active account in another configuration +directory. Authentication failure or a mismatch blocks the action; there is no +fallback. Git operations use the selected account's credential helper and +signing configuration. Use HTTPS GitHub remotes; SSH transport and interactive +credential prompts are disabled for this account runner. + +`agent github pending` binds each GitHub activity to its session's configured +account. `execution_account` records the account name and expected login. A +retry with a different binding is refused. Reusing an existing PR also requires +its author to match; a PR authored by another account is not silently adopted. + +The same session selection applies to assignment scans, PR-merge scans, +supervised issue reads, and the `pushed`/`mergeable` steps of `agent run`. +Account selection is script configuration, never an instruction taken from an +issue body or a model's activity payload. Configure existing sessions explicitly +before enabling these operations after upgrading. + +Hub pairing and event ownership are separate: using a second GitHub execution +account does not re-pair the device or change ownership of its rows. + +This document covers GitHub execution. Configurable AI accounts and roles are +part of the [empty-default requirement](../DESIGN.md#198-configuration-starts-empty); +they are not implemented by this manifest. diff --git a/src/agent_cli/github_accounts.py b/src/agent_cli/github_accounts.py new file mode 100644 index 0000000..145b19c --- /dev/null +++ b/src/agent_cli/github_accounts.py @@ -0,0 +1,184 @@ +"""Device-local GitHub account bindings for static executors. + +No configured account means no GitHub execution. Credentials stay in each +account's gh configuration directory, never in activities or this manifest. +""" + +from __future__ import annotations + +import json +import re +from collections.abc import Callable +from dataclasses import dataclass +from pathlib import Path +from typing import Any + +from .runtime import Completed +from .store import StoreError + +Runner = Callable[[list[str]], Completed] +CONFIG_FILE = "github-accounts.json" +_LOGIN = re.compile(r"[A-Za-z0-9](?:[A-Za-z0-9-]*[A-Za-z0-9])?") +_TOKEN_ENV = ( + "GH_TOKEN", "GITHUB_TOKEN", "GH_ENTERPRISE_TOKEN", "GITHUB_ENTERPRISE_TOKEN", + "GIT_ASKPASS", "SSH_ASKPASS", +) + + +class AccountError(StoreError): + """Configuration or authentication does not authorize this execution.""" + + +def _text(value: Any, field: str) -> str: + if not isinstance(value, str) or not value.strip() or "\x00" in value or "\n" in value: + raise AccountError(f"{field} must be a non-empty single-line string") + return value + + +@dataclass(frozen=True) +class Account: + name: str + login: str + gh_config_dir: str + git_identity: dict[str, str] | None = None + command_prefix: tuple[str, ...] = () + worktree_paths: tuple[tuple[str, str], ...] = () + + def runner(self, base: Runner, *, require_git: bool = False) -> Runner: + if require_git and self.git_identity is None: + raise AccountError(f"Git identity is not configured for account {self.name}") + + def scoped(argv: list[str]) -> Completed: + if not argv or argv[0] not in {"gh", "git"}: + raise AccountError("GitHub account runner accepts only gh and git") + prefix = ["env"] + for key in _TOKEN_ENV: + prefix.extend(["-u", key]) + prefix.extend([ + f"GH_CONFIG_DIR={self.gh_config_dir}", "GH_HOST=github.com", "GH_PROMPT_DISABLED=1", + "GIT_TERMINAL_PROMPT=0", "GIT_SSH_COMMAND=false", + ]) + command = list(argv) + if argv[0] == "git": + identity = self.git_identity + if identity is None: + raise AccountError(f"Git identity is not configured for account {self.name}") + prefix.extend([ + "GIT_CONFIG_COUNT=0", + f"GIT_AUTHOR_NAME={identity['name']}", + f"GIT_AUTHOR_EMAIL={identity['email']}", + f"GIT_COMMITTER_NAME={identity['name']}", + f"GIT_COMMITTER_EMAIL={identity['email']}", + ]) + git_args = list(argv[1:]) + if "-C" in git_args: + index = git_args.index("-C") + 1 + if index >= len(git_args): + raise AccountError("git -C requires a worktree path") + path = Path(git_args[index]) + if not path.is_absolute() or ".." in path.parts: + raise AccountError("Git worktree path must be absolute without parent traversal") + for source, target in sorted(self.worktree_paths, key=lambda pair: len(pair[0]), reverse=True): + if path.is_relative_to(source): + git_args[index] = str(Path(target) / path.relative_to(source)) + break + command = [ + "git", "-c", "core.askPass=", "-c", "http.extraHeader=", + "-c", "http.https://github.com/.extraHeader=", "-c", "credential.helper=", + "-c", "credential.helper=!gh auth git-credential", + "-c", f"user.name={identity['name']}", "-c", f"user.email={identity['email']}", + "-c", "commit.gpgsign=true", "-c", f"gpg.format={identity['signing_format']}", + "-c", f"user.signingkey={identity['signing_key']}", *git_args, + ] + return base([*self.command_prefix, *prefix, *command]) + + try: + result = scoped(["gh", "api", "user", "--jq", ".login"]) + except OSError as exc: + raise AccountError(f"Cannot authenticate account {self.name}") from exc + # Do not copy authentication stderr (which may contain credentials) into the store. + if result.returncode != 0: + raise AccountError(f"Authentication failed for account {self.name}; no fallback") + if result.stdout.strip().casefold() != self.login.casefold(): + raise AccountError(f"Authenticated login does not match account {self.name}; no fallback") + return scoped + + +@dataclass(frozen=True) +class Accounts: + accounts: dict[str, Account] + sessions: dict[str, str] + + def for_session(self, session_id: str) -> Account: + name = self.sessions.get(session_id) + if name is None: + raise AccountError(f"No GitHub account configured for session {session_id}") + return self.accounts[name] + + +def load_accounts(home: Path) -> Accounts: + path = home / CONFIG_FILE + if not path.exists(): + return Accounts({}, {}) + try: + raw = json.loads(path.read_text(encoding="utf-8")) + except (OSError, UnicodeError, ValueError) as exc: + raise AccountError(f"Cannot read {CONFIG_FILE}") from exc + if not isinstance(raw, dict) or set(raw) - {"accounts", "sessions"}: + raise AccountError(f"{CONFIG_FILE} accepts only accounts and sessions") + accounts_raw = raw.get("accounts") + sessions_raw = raw.get("sessions") + if accounts_raw is None: + accounts_raw = {} + if sessions_raw is None: + sessions_raw = {} + if not isinstance(accounts_raw, dict) or not isinstance(sessions_raw, dict): + raise AccountError("accounts and sessions must be objects or null") + accounts: dict[str, Account] = {} + for name, entry in accounts_raw.items(): + _text(name, "account name") + if not isinstance(entry, dict) or set(entry) - {"login", "gh_config_dir", "git", "command_prefix", "worktree_paths"}: + raise AccountError(f"Invalid account fields for {name}") + login = _text(entry.get("login"), "login") + if _LOGIN.fullmatch(login) is None: + raise AccountError(f"Invalid login for {name}") + config_dir = _text(entry.get("gh_config_dir"), "gh_config_dir") + if not Path(config_dir).is_absolute(): + raise AccountError("gh_config_dir must be an absolute path") + identity = entry.get("git") + if identity is not None: + keys = {"name", "email", "signing_format", "signing_key"} + if not isinstance(identity, dict) or set(identity) != keys: + raise AccountError(f"git for {name} requires name, email, signing_format, signing_key") + identity = {key: _text(value, f"git.{key}") for key, value in identity.items()} + if identity["signing_format"] not in {"ssh", "openpgp", "x509"}: + raise AccountError(f"Invalid signing_format for {name}") + command_prefix = entry.get("command_prefix") + if command_prefix is None: + command_prefix = [] + if not isinstance(command_prefix, list): + raise AccountError("command_prefix must be an argv array") + command_prefix = tuple(_text(item, "command_prefix item") for item in command_prefix) + mappings = entry.get("worktree_paths") + if mappings is None: + mappings = {} + if not isinstance(mappings, dict): + raise AccountError("worktree_paths must map absolute host paths to executor paths") + paths = [] + for source, target in mappings.items(): + source = _text(source, "worktree source") + target = _text(target, "worktree target") + if not Path(source).is_absolute() or not Path(target).is_absolute(): + raise AccountError("worktree paths must be absolute") + if ".." in Path(source).parts or ".." in Path(target).parts: + raise AccountError("worktree paths must not contain parent traversal") + paths.append((str(Path(source)), str(Path(target)))) + accounts[name] = Account(name, login, config_dir, identity, command_prefix, tuple(paths)) + sessions: dict[str, str] = {} + for session, name in sessions_raw.items(): + _text(session, "session id") + _text(name, "session account") + if name not in accounts: + raise AccountError(f"Session {session} references an unknown account") + sessions[session] = name + return Accounts(accounts, sessions) diff --git a/src/agent_cli/github_act.py b/src/agent_cli/github_act.py index e114148..629d8a9 100644 --- a/src/agent_cli/github_act.py +++ b/src/agent_cli/github_act.py @@ -9,6 +9,7 @@ from .runtime import Completed from .store import Store +from .github_accounts import AccountError, load_accounts Runner = Callable[[list[str]], Completed] @@ -174,7 +175,7 @@ def _flatten_comment_pages(data: Any) -> list[Any]: raise _GhError("comments api has unexpected shape") -def _run_pr_open(store: Store, runner: Runner, row: dict[str, Any]) -> str: +def _run_pr_open(store: Store, runner: Runner, row: dict[str, Any], *, expected_login: str | None = None) -> str: rid = str(row["id"]) payload = row.get("payload") if not isinstance(payload, dict): @@ -202,7 +203,7 @@ def _run_pr_open(store: Store, runner: Runner, row: dict[str, Any]) -> str: "--repo", repo, "--json", - "number,url,state,isDraft", + "number,url,state,isDraft" + (",author" if expected_login else ""), ] try: completed = runner(view_argv) @@ -228,6 +229,11 @@ def _run_pr_open(store: Store, runner: Runner, row: dict[str, Any]) -> str: raise _GhError("gh output is not a JSON object or array") viewed = data if isinstance(viewed, dict): + if expected_login: + author = viewed.get("author") + login = author.get("login") if isinstance(author, dict) else None + if not isinstance(login, str) or login.casefold() != expected_login.casefold(): + raise _GhError("Existing pull request author differs from the configured account") state = str(viewed.get("state") or "").upper() number = _as_int(viewed.get("number")) url = viewed.get("url") @@ -563,16 +569,25 @@ def scan_github(store: Store, runner: Runner) -> list[str]: lines: list[str] = [] for row in store.pending_work(): typ = row.get("type") + if typ not in {"pr.open", "issue.write", "comment.post", "review.post"}: + continue try: + account = load_accounts(store.home).for_session(str(row.get("session_id") or "")) + binding = {"account": account.name, "login": account.login.casefold()} + if row.get("execution_account") is not None and row["execution_account"] != binding: + raise AccountError("Activity account binding changed; refusing account switch") + row = dict(row, execution_account=binding) + store.write("activity", "update", row["id"], _strip(row)) + scoped_runner = account.runner(runner) if typ == "pr.open": - lines.append(_run_pr_open(store, runner, row)) + lines.append(_run_pr_open(store, scoped_runner, row, expected_login=account.login)) elif typ == "issue.write": - lines.append(_run_issue_write(store, runner, row)) + lines.append(_run_issue_write(store, scoped_runner, row)) elif typ == "comment.post": - lines.append(_run_comment_post(store, runner, row)) + lines.append(_run_comment_post(store, scoped_runner, row)) elif typ == "review.post": - lines.append(_run_review_post(store, runner, row)) - except Exception as exc: # noqa: BLE001 — per-row isolation + lines.append(_run_review_post(store, scoped_runner, row)) + except (Exception, AccountError) as exc: # noqa: BLE001 — per-row isolation rid = str(row.get("id") or "?") _mark(store, row, status="error", error=str(exc)) label = typ if isinstance(typ, str) else "activity" diff --git a/src/agent_cli/main.py b/src/agent_cli/main.py index edcb0ca..9391196 100644 --- a/src/agent_cli/main.py +++ b/src/agent_cli/main.py @@ -2642,10 +2642,12 @@ def cmd_run(args: list[str]) -> None: if step.key == "pushed": cwd = _resolve_run_cwd(args) from .git_act import GitActError, push_branch + from .github_accounts import load_accounts try: + account = load_accounts(store.home).for_session(str(snap.get("session_id") or "")) sha = push_branch( - cwd=cwd, runner=lambda argv: _exec_argv(argv, cwd=cwd) + cwd=cwd, runner=account.runner(lambda argv: _exec_argv(argv, cwd=cwd), require_git=True) ) except GitActError as exc: die(str(exc)) @@ -2661,12 +2663,14 @@ def cmd_run(args: list[str]) -> None: if step.key == "mergeable": cwd = _resolve_run_cwd(args) from .git_act import GitActError, measure_mergeable + from .github_accounts import load_accounts try: expected = str(snap.get("head_sha") or head or "").strip() or None + account = load_accounts(store.home).for_session(str(snap.get("session_id") or "")) evidence = measure_mergeable( cwd=cwd, - runner=lambda argv: _exec_argv(argv, cwd=cwd), + runner=account.runner(lambda argv: _exec_argv(argv, cwd=cwd)), expected_head=expected, ) except GitActError as exc: diff --git a/src/agent_cli/skills/error-fix/SKILL.md b/src/agent_cli/skills/error-fix/SKILL.md index bf09ace..460e4db 100644 --- a/src/agent_cli/skills/error-fix/SKILL.md +++ b/src/agent_cli/skills/error-fix/SKILL.md @@ -24,6 +24,16 @@ rules live in DESIGN.md §§14–15, §19, and §21. ## Loop +The static script starts implementation and review lanes, starts the implementer +again for improvements, executes tests, and performs all GitHub communication. +Model lanes never start subagents or execute tests themselves. Executor commands +below belong to the script. See +[DESIGN.md §19.1](../../../../DESIGN.md#191-responsibility-split). + +Model lanes never start monitors, poll, or wait for logs, CI, or PR events. +Return the current result or blocker to the script. Event detection and any +subsequent notification that provides useful model work belong to the script. + 1. A **script** on this device queries a configured log source, redacts, fingerprints, and inserts or enriches `activity.type=error.seen` on this session. First insert knocks `da ist Post id `. Enrichment never diff --git a/src/agent_cli/skills/pr-review/SKILL.md b/src/agent_cli/skills/pr-review/SKILL.md index d5b8f77..810058d 100644 --- a/src/agent_cli/skills/pr-review/SKILL.md +++ b/src/agent_cli/skills/pr-review/SKILL.md @@ -17,6 +17,16 @@ replace it with a second store or a side process. ## Gates +The static script starts every review lane and any implementation pass needed +to address findings. Reviewers return findings to that script; they never start +subagents or interact with GitHub. Gate recording and GitHub publication below +are script operations. See +[DESIGN.md §19.1](../../../../DESIGN.md#191-responsibility-split). + +Do not start a monitor, poll GitHub, or wait for CI or another review. Return +findings or blockers to the script when the review work is exhausted. The +script monitors events and informs a lane when further work is useful. + Two dimensions (quality, logic) and two vendor stages (`grok-pr`, then `codex-pr`). Codex stages run only if both grok dimensions are `approved`. diff --git a/src/agent_cli/skills/review-loop/SKILL.md b/src/agent_cli/skills/review-loop/SKILL.md index 2c12be5..54e8f95 100644 --- a/src/agent_cli/skills/review-loop/SKILL.md +++ b/src/agent_cli/skills/review-loop/SKILL.md @@ -13,6 +13,16 @@ Without this skill, implementer and reviewer `agent agent` commands refuse. ## Loop +The static script owns this loop: it starts the implementer, starts the reviewer, +and starts the implementer again for improvements. The commands below are script +operations, not instructions for a model to launch another lane. Neither role +may start subagents, execute tests, or access GitHub. See +[DESIGN.md §19.1](../../../../DESIGN.md#191-responsibility-split). + +Neither lane starts monitors or waits/polls for tests, CI, or another lane. +Return the implementation result, findings, or blocker to the script. It owns +monitoring and informs the model when an event provides useful work. + No round cap. Repeat until the reviewer sets `approved` or the implementer is `blocked`. diff --git a/src/agent_cli/skills/session-store/SKILL.md b/src/agent_cli/skills/session-store/SKILL.md index 79c8fa5..dce733c 100644 --- a/src/agent_cli/skills/session-store/SKILL.md +++ b/src/agent_cli/skills/session-store/SKILL.md @@ -8,6 +8,17 @@ description: >- # Session store +All GitHub communication, tests, and lane/subagent starts belong to static +scripts, never to model lanes. For the required assignment workflow, the script +accepts the issue and confirms acceptance on GitHub before it starts the +implementation lane. See +[DESIGN.md §19.7](../../../../DESIGN.md#197-issue-assignment-to-human-merge), which +separates this requirement from the existing watcher behavior described below. + +Never start a monitor, poll status, or wait for CI or other events. Return your +result or blocker to the script when no work remains. Scripts own monitoring +and inform the model when an observed event provides useful work. + Install this package locally and put `agent` on `PATH`. There is no second binary. diff --git a/src/agent_cli/skills/spine/SKILL.md b/src/agent_cli/skills/spine/SKILL.md index a6915b4..4a2a89a 100644 --- a/src/agent_cli/skills/spine/SKILL.md +++ b/src/agent_cli/skills/spine/SKILL.md @@ -20,6 +20,16 @@ Without spine, `task`, `checklist`, `round`, `check`, `work`, `allow`, `next`, ## One open step +The static script owns execution and progression. A model lane must not invoke +`agent run` to launch another lane, run tests, or perform GitHub operations. +Tests and lane starts in the workflow below are script responsibilities; models +return their implementation or review results. See +[DESIGN.md §19.1](../../../../DESIGN.md#191-responsibility-split). + +Model lanes never start monitors or wait/poll for checks, reviews, or CI. Return +the current result or blocker to the script; it observes events and informs a +lane when further work is useful. + `agent next`, `agent close-step`, and `agent run` are the spine. Do not skip keys. Quality and logic of the same vendor stage may be open together. `close-step` applies chain guards, then writes via `checklist set`. diff --git a/src/agent_cli/supervise.py b/src/agent_cli/supervise.py index 4bf0c18..fac37db 100644 --- a/src/agent_cli/supervise.py +++ b/src/agent_cli/supervise.py @@ -214,6 +214,10 @@ def enqueue_assigned( existing = pending_repo_number(store, session_id, repo, number) if existing is not None: return existing + from .github_accounts import load_accounts + + account = load_accounts(store.home).for_session(session_id) + runner = account.runner(runner) now = utcnow() _ensure_assigned_session(store, session_id, now) url = f"https://github.com/{repo}/issues/{number}" diff --git a/src/agent_cli/watch.py b/src/agent_cli/watch.py index 99fd06c..0aa34fd 100644 --- a/src/agent_cli/watch.py +++ b/src/agent_cli/watch.py @@ -116,6 +116,13 @@ def scan_merged( if _already_merged(store, session_id, repo, number): continue try: + from .github_accounts import AccountError, load_accounts + + account = load_accounts(store.home).for_session(session_id) + binding = {"account": account.name, "login": account.login.casefold()} + if row.get("execution_account") is not None and row["execution_account"] != binding: + raise AccountError("Activity account binding changed; refusing account switch") + scoped_runner = account.runner(runner) info = _gh( [ "gh", @@ -127,7 +134,7 @@ def scan_merged( "--json", "state,mergedAt,mergeCommit,url,number", ], - runner, + scoped_runner, ) except (StoreError, json.JSONDecodeError): skipped += 1 @@ -229,19 +236,6 @@ def _parse_gh_time(raw: str) -> datetime: return datetime.fromisoformat(raw.replace("Z", "+00:00")) -def _paired_login(store: Store, runner: Callable[[list[str]], Completed]) -> str: - paired = store.meta("github_login") - if not isinstance(paired, str) or paired == "": - raise StoreError("paired github_login is missing") - user = _gh(["gh", "api", "user"], runner) - login = user.get("login") - if not isinstance(login, str) or login == "": - raise StoreError("gh api user did not return a string login") - if login.lower() != paired.lower(): - raise StoreError(f"gh login {login} does not match paired github_login {paired}") - return paired.lower() - - def _latest_assigned_at(store: Store, repo: str, number: int) -> datetime | None: repo_key = repo.lower() latest: datetime | None = None @@ -330,7 +324,11 @@ def scan_assigned( ) -> tuple[list[str], int]: """Insert issue.assigned for allowlisted open issues newly assigned to this login.""" repos, sid = load_watch_config(store.home) - login = _paired_login(store, runner) + from .github_accounts import load_accounts + + account = load_accounts(store.home).for_session(sid) + runner = account.runner(runner) + login = account.login.casefold() cursor = store.sync_get("assigned_watch_since") if cursor is None: store.sync_set("assigned_session_id", sid) diff --git a/tests/github_support.py b/tests/github_support.py new file mode 100644 index 0000000..3bdf1ae --- /dev/null +++ b/tests/github_support.py @@ -0,0 +1,44 @@ +"""Explicit accounts for existing protocol tests using a mocked gh transport. + +Account isolation and missing-configuration behavior are tested without this +adapter in test_github_accounts.py. This adapter preserves the older protocol +fixtures while supplying their newly required account/authentication context. +""" +import json +from pathlib import Path +from agent_cli.runtime import Completed + + +def configure_accounts(home: Path, sessions, *, login='alice'): + (home / 'github-accounts.json').write_text(json.dumps({ + 'accounts': {'test': {'login': login, 'gh_config_dir': '/test/gh', 'git': { + 'name': 'Test Worker', 'email': 'test@example.com', + 'signing_format': 'ssh', 'signing_key': '/test/key', + }}}, + 'sessions': {sid: 'test' for sid in sessions}, + })) + + +def transport(runner, *, login='alice', authenticate=False): + def scoped(argv): + assert argv[0] == 'env' + assert 'GH_CONFIG_DIR=/test/gh' in argv + command = argv[argv.index('gh'):] + if command == ['gh', 'api', 'user', '--jq', '.login']: + if not authenticate: + return Completed(0, login, '') + result = runner(['gh', 'api', 'user']) + if result.returncode: + return result + return Completed(0, json.loads(result.stdout)['login'], result.stderr) + result = runner(command) + if command[:3] == ['gh', 'pr', 'view'] and result.returncode == 0: + try: + data = json.loads(result.stdout) + except ValueError: + return result + if isinstance(data, dict): + data.setdefault('author', {'login': login}) + return Completed(result.returncode, json.dumps(data), result.stderr) + return result + return scoped diff --git a/tests/test_github_accounts.py b/tests/test_github_accounts.py new file mode 100644 index 0000000..23b2f0e --- /dev/null +++ b/tests/test_github_accounts.py @@ -0,0 +1,281 @@ +from __future__ import annotations + +import json +import os +import sys +from pathlib import Path + +import pytest + +from agent_cli.github_accounts import Account, AccountError, load_accounts +from agent_cli.github_act import scan_github +from agent_cli.runtime import Completed, run_argv +from agent_cli.store import Store +from agent_cli.watch import scan_assigned, scan_merged + + +def configure(home: Path, *, sessions=None) -> None: + (home / 'github-accounts.json').write_text(json.dumps({ + 'accounts': { + 'one': {'login': 'WorkerOne', 'gh_config_dir': '/accounts/one'}, + 'two': {'login': 'WorkerTwo', 'gh_config_dir': '/accounts/two'}, + }, + 'sessions': sessions if sessions is not None else {'s1': 'one', 's2': 'two'}, + })) + + +def pending(store: Store, sid: str, aid: str) -> None: + store.write('session', 'insert', sid, {'id': sid, 'kind': 'runner', 'status': 'active'}) + store.write('activity', 'insert', aid, { + 'id': aid, 'session_id': sid, 'type': 'pr.open', 'execution_status': 'pending', + 'payload': {'repo': 'owner/repo', 'head': f'feature-{sid}', 'title': 'Fix', 'base': 'develop'}, + }) + + +def unpack(argv: list[str]) -> tuple[str, list[str]]: + profile = next(s.split('=', 1)[1] for s in argv if s.startswith('GH_CONFIG_DIR=')) + return profile, argv[argv.index('gh'):] + + +@pytest.mark.no_pg +@pytest.mark.parametrize('data', [None, {}, {'accounts': None, 'sessions': None}, {'accounts': {}, 'sessions': {}}]) +def test_installation_has_no_account(tmp_path, data): + if data is not None: + (tmp_path / 'github-accounts.json').write_text(json.dumps(data)) + accounts = load_accounts(tmp_path) + assert accounts.accounts == {} + with pytest.raises(AccountError, match='No GitHub account configured'): + accounts.for_session('s1') + + +def test_unconfigured_executor_does_not_call_github(tmp_path): + store = Store(tmp_path) + pending(store, 's1', 'a1') + def forbidden(argv): + pytest.fail('Unconfigured execution must not access GitHub') + assert scan_github(store, forbidden) == ['pr.open a1 error'] + assert 'No GitHub account configured' in store.row('activity', 'a1')['execution_error'] + + +def test_two_sessions_use_separate_accounts(tmp_path): + configure(tmp_path) + store = Store(tmp_path) + pending(store, 's1', 'a1') + pending(store, 's2', 'a2') + calls = [] + def runner(argv): + profile, command = unpack(argv) + calls.append((profile, command)) + number = 1 if profile == '/accounts/one' else 2 + if command[:3] == ['gh', 'api', 'user']: + return Completed(0, 'WorkerOne' if number == 1 else 'WorkerTwo', '') + if command[:3] == ['gh', 'pr', 'view']: + return Completed(1, '', 'no pull requests found') + assert command[:3] == ['gh', 'pr', 'create'] + assert command[command.index('--head') + 1] == f'feature-s{number}' + assert '--draft' in command + return Completed(0, f'https://github.com/owner/repo/pull/{number}', '') + assert len(scan_github(store, runner)) == 2 + assert store.row('activity', 'a1')['execution_account'] == {'account': 'one', 'login': 'workerone'} + assert store.row('activity', 'a2')['execution_account'] == {'account': 'two', 'login': 'workertwo'} + assert store.row('activity', 'a1')['result']['number'] == 1 + assert store.row('activity', 'a2')['result']['number'] == 2 + assert len(calls) == 6 + + +@pytest.mark.parametrize('auth', [Completed(0, 'WrongAccount', ''), Completed(1, '', 'PRIVATE_AUTH_DETAIL')]) +def test_wrong_or_failed_login_never_falls_back(tmp_path, auth): + configure(tmp_path) + store = Store(tmp_path) + pending(store, 's1', 'a1') + calls = [] + def runner(argv): + profile, command = unpack(argv) + calls.append(profile) + assert command[:3] == ['gh', 'api', 'user'] + return auth + assert scan_github(store, runner) == ['pr.open a1 error'] + assert calls == ['/accounts/one'] + error = store.row('activity', 'a1')['execution_error'] + assert 'no fallback' in error + assert 'PRIVATE_AUTH_DETAIL' not in error + + +def test_retry_cannot_change_account(tmp_path): + configure(tmp_path) + store = Store(tmp_path) + pending(store, 's1', 'a1') + scan_github(store, lambda argv: Completed(1, '', 'offline')) + row = store.row('activity', 'a1') + row['execution_status'] = 'pending' + store.write('activity', 'update', 'a1', row) + configure(tmp_path, sessions={'s1': 'two'}) + def forbidden(argv): + pytest.fail('Changed binding must fail before authentication') + scan_github(store, forbidden) + assert 'binding changed' in store.row('activity', 'a1')['execution_error'] + + +def test_existing_pr_from_another_account_is_not_adopted(tmp_path): + configure(tmp_path) + store = Store(tmp_path) + pending(store, 's1', 'a1') + def runner(argv): + _, command = unpack(argv) + if command[:3] == ['gh', 'api', 'user']: + return Completed(0, 'WorkerOne', '') + assert command[:3] == ['gh', 'pr', 'view'] + return Completed(0, json.dumps({'number': 4, 'url': 'https://github.com/owner/repo/pull/4', + 'state': 'OPEN', 'isDraft': True, 'author': {'login': 'WorkerTwo'}}), '') + scan_github(store, runner) + assert 'author differs' in store.row('activity', 'a1')['execution_error'] + + +@pytest.mark.no_pg +def test_child_environment_is_isolated_without_mutating_parent(tmp_path, monkeypatch): + gh = tmp_path / 'gh' + gh.write_text(f'#!{sys.executable}\n' + '''import os, sys +assert all(k not in os.environ for k in ('GH_TOKEN', 'GITHUB_TOKEN', 'GH_ENTERPRISE_TOKEN', 'GITHUB_ENTERPRISE_TOKEN')) +assert os.environ['GH_HOST'] == 'github.com' +print(os.path.basename(os.environ['GH_CONFIG_DIR'])) +''') + gh.chmod(0o755) + monkeypatch.setenv('PATH', str(tmp_path) + os.pathsep + os.environ['PATH']) + monkeypatch.setenv('GH_TOKEN', 'parent-only') + monkeypatch.setenv('GITHUB_TOKEN', 'parent-only') + monkeypatch.setenv('GH_CONFIG_DIR', '/parent/config') + monkeypatch.setenv('GH_HOST', 'wrong.example') + one = Account('one', 'WorkerOne', '/accounts/WorkerOne').runner(run_argv) + two = Account('two', 'WorkerTwo', '/accounts/WorkerTwo').runner(run_argv) + assert one(['gh', 'api', 'user']).stdout.strip() == 'WorkerOne' + assert two(['gh', 'api', 'user']).stdout.strip() == 'WorkerTwo' + assert one(['gh', 'api', 'user']).stdout.strip() == 'WorkerOne' + assert os.environ['GH_TOKEN'] == 'parent-only' + assert os.environ['GH_CONFIG_DIR'] == '/parent/config' + + +@pytest.mark.no_pg +def test_git_requires_explicit_identity_and_uses_scoped_helper(): + account = Account('one', 'WorkerOne', '/accounts/one') + with pytest.raises(AccountError, match='Git identity is not configured'): + account.runner(lambda argv: pytest.fail('No auth before missing identity is reported'), require_git=True) + identity = {'name': 'Worker One', 'email': 'one@example.com', 'signing_key': '/keys/one', 'signing_format': 'ssh'} + calls = [] + def runner(argv): + calls.append(argv) + return Completed(0, 'WorkerOne', '') + scoped = Account('one', 'WorkerOne', '/accounts/one', identity).runner(runner, require_git=True) + scoped(['git', '-C', '/work', 'push', 'origin', 'feature']) + command = calls[-1] + assert 'GIT_AUTHOR_EMAIL=one@example.com' in command + assert 'GIT_COMMITTER_EMAIL=one@example.com' in command + assert 'user.signingkey=/keys/one' in command + assert 'credential.helper=' in command + assert 'credential.helper=!gh auth git-credential' in command + assert 'GIT_TERMINAL_PROMPT=0' in command + assert 'GIT_SSH_COMMAND=false' in command + assert 'GIT_CONFIG_COUNT=0' in command + assert 'http.https://github.com/.extraHeader=' in command + + +@pytest.mark.no_pg +@pytest.mark.parametrize('change', ['unknown-session-account', 'relative-directory', 'token-field', 'partial-git', 'invalid-json']) +def test_invalid_configuration_does_not_choose_defaults(tmp_path, change): + configure(tmp_path) + path = tmp_path / 'github-accounts.json' + data = json.loads(path.read_text()) + if change == 'unknown-session-account': data['sessions']['s1'] = 'missing' + elif change == 'relative-directory': data['accounts']['one']['gh_config_dir'] = './relative' + elif change == 'token-field': data['accounts']['one']['token'] = 'not-allowed' + elif change == 'partial-git': data['accounts']['one']['git'] = {'name': 'Incomplete'} + path.write_text('{' if change == 'invalid-json' else json.dumps(data)) + with pytest.raises(AccountError): load_accounts(tmp_path) + + +def test_assignment_uses_execution_account_not_hub_pairing(tmp_path): + configure(tmp_path, sessions={'assigned': 'two'}) + (tmp_path / 'watch.json').write_text(json.dumps({'assigned_repos': ['owner/repo']})) + store = Store(tmp_path) + store.set_meta('github_login', 'HubOwner') + store.sync_set('assigned_watch_since', '2026-01-01T00:00:00Z') + def runner(argv): + profile, command = unpack(argv) + assert profile == '/accounts/two' + if command[:3] == ['gh', 'api', 'user']: return Completed(0, 'WorkerTwo', '') + assert command[:3] == ['gh', 'issue', 'list'] + assert command[command.index('--assignee') + 1] == 'workertwo' + return Completed(0, '[]', '') + assert scan_assigned(store, runner, now='2026-09-06T00:00:00Z') == ([], 0) + assert store.meta('github_login') == 'HubOwner' + + +@pytest.mark.no_pg +def test_container_account_keeps_credentials_and_signing_in_its_executor(tmp_path): + data = { + 'accounts': {'container': { + 'login': 'ContainerWorker', 'gh_config_dir': '/home/worker/.config/gh', + 'git': {'name': 'Worker', 'email': 'worker@example.com', 'signing_format': 'ssh', 'signing_key': '/home/worker/.ssh/key'}, + 'command_prefix': ['docker', 'exec', '-i', 'worker-container'], + 'worktree_paths': {'/srv/worker/data': '/data'}, + }}, + 'sessions': {'s1': 'container'}, + } + (tmp_path / 'github-accounts.json').write_text(json.dumps(data)) + calls = [] + def runner(argv): + calls.append(argv) + assert argv[:5] == ['docker', 'exec', '-i', 'worker-container', 'env'] + return Completed(0, 'ContainerWorker', '') + scoped = load_accounts(tmp_path).for_session('s1').runner(runner, require_git=True) + scoped(['git', '-C', '/srv/worker/data/repo', 'push', 'origin', 'feature']) + command = calls[-1] + assert command[command.index('-C') + 1] == '/data/repo' + assert 'GH_CONFIG_DIR=/home/worker/.config/gh' in command + assert 'user.signingkey=/home/worker/.ssh/key' in command + scoped(['git', '-C', '/srv/worker/database/repo', 'status']) + assert calls[-1][calls[-1].index('-C') + 1] == '/srv/worker/database/repo' + with pytest.raises(AccountError, match='parent traversal'): + scoped(['git', '-C', '/srv/worker/data/../other', 'status']) + + +@pytest.mark.no_pg +@pytest.mark.parametrize('key,value', [('command_prefix', False), ('worktree_paths', []), ('command_prefix', ['']), ('worktree_paths', {'/a': '/b/../c'})]) +def test_invalid_executor_configuration_is_rejected(tmp_path, key, value): + configure(tmp_path) + path = tmp_path / 'github-accounts.json' + data = json.loads(path.read_text()) + data['accounts']['one'][key] = value + path.write_text(json.dumps(data)) + with pytest.raises(AccountError): load_accounts(tmp_path) + + +def test_failed_account_does_not_stop_other_accounts(tmp_path): + configure(tmp_path) + store = Store(tmp_path) + pending(store, 's1', 'a1') + pending(store, 's2', 'a2') + def runner(argv): + profile, command = unpack(argv) + if profile == '/accounts/one': + assert command[:3] == ['gh', 'api', 'user'] + return Completed(1, '', 'offline') + if command[:3] == ['gh', 'api', 'user']: return Completed(0, 'WorkerTwo', '') + if command[:3] == ['gh', 'pr', 'view']: return Completed(1, '', 'no pull requests found') + assert command[:3] == ['gh', 'pr', 'create'] + return Completed(0, 'https://github.com/owner/repo/pull/2', '') + lines = scan_github(store, runner) + assert set(lines) == {'pr.open a1 error', 'pr.open a2 done number=2'} + assert store.row('activity', 'a1')['execution_account']['account'] == 'one' + assert store.row('activity', 'a2')['execution_account']['account'] == 'two' + + +def test_merge_watch_refuses_an_account_change(tmp_path): + configure(tmp_path) + store = Store(tmp_path) + pending(store, 's1', 'a1') + row = store.row('activity', 'a1') + row.update(execution_status='done', execution_account={'account': 'two', 'login': 'workertwo'}, + result={'repo': 'owner/repo', 'number': 1, 'url': 'https://github.com/owner/repo/pull/1'}) + store.write('activity', 'update', 'a1', row) + def forbidden(argv): pytest.fail('Changed binding must not reach GitHub') + assert scan_merged(store, forbidden) == ([], 1) diff --git a/tests/test_github_act.py b/tests/test_github_act.py index b2c1e52..5666a18 100644 --- a/tests/test_github_act.py +++ b/tests/test_github_act.py @@ -6,12 +6,21 @@ import pytest -from agent_cli.github_act import ACTIVITY_MARKER, scan_github +from agent_cli.github_act import ACTIVITY_MARKER, scan_github as account_scan_github from agent_cli.main import main from agent_cli.runtime import Completed from agent_cli.store import Store + +from github_support import configure_accounts, transport + + +def scan_github(store, runner): + configure_accounts(store.home, [r['id'] for r in store.rows('session')], login='theo-vane') + return account_scan_github(store, transport(runner, login='theo-vane')) + + def run(home: Path, argv: list[str]) -> None: import os diff --git a/tests/test_run.py b/tests/test_run.py index efdbf54..a37b990 100644 --- a/tests/test_run.py +++ b/tests/test_run.py @@ -9,6 +9,7 @@ from agent_cli.runtime import Completed from agent_cli.store import Store from test_cli import _last_agent_id, _last_task_id, run +from github_support import configure_accounts def _store(home: Path) -> Store: @@ -453,6 +454,7 @@ def test_run_pushed_calls_push_branch( ) -> None: tid = _bootstrap_implement(tmp_path, capsys) _advance_to_pushed(tmp_path, tid, capsys, monkeypatch) + configure_accounts(tmp_path, ["sess-1"], login="ok") called = {"n": 0} @@ -582,6 +584,7 @@ def test_run_mergeable_after_gates( ) -> None: tid = _bootstrap_resolve(tmp_path, capsys) _advance_to_pushed(tmp_path, tid, capsys, monkeypatch) + configure_accounts(tmp_path, ["sess-1"], login="ok") push_called = {"n": 0} diff --git a/tests/test_supervise.py b/tests/test_supervise.py index 32e2ca9..e74dabf 100644 --- a/tests/test_supervise.py +++ b/tests/test_supervise.py @@ -11,12 +11,21 @@ ANSWER_NO, ANSWER_YES, QUESTION_DONE, - enqueue_assigned, + enqueue_assigned as account_enqueue_assigned, parse_closed_answer, tick, ) + +from github_support import configure_accounts, transport + + +def enqueue_assigned(store, session_id, repo, number, runner): + configure_accounts(store.home, [session_id]) + return account_enqueue_assigned(store, session_id, repo, number, transport(runner)) + + class FakeRuntime(Runtime): def __init__( self, diff --git a/tests/test_watch.py b/tests/test_watch.py index ce0bb5e..825966b 100644 --- a/tests/test_watch.py +++ b/tests/test_watch.py @@ -12,11 +12,30 @@ dispatch_assigned, load_watch_config, pending_assigned, - scan_assigned, - scan_merged, + scan_assigned as account_scan_assigned, + scan_merged as account_scan_merged, ) + +from github_support import configure_accounts, transport + + +def scan_assigned(store, runner, *, now): + path = store.home / 'watch.json' + try: + sid = json.loads(path.read_text()).get('session_id', 'assigned') + except (OSError, ValueError): + sid = 'assigned' + configure_accounts(store.home, [sid]) + return account_scan_assigned(store, transport(runner, authenticate=True), now=now) + + +def scan_merged(store, runner): + configure_accounts(store.home, [r['id'] for r in store.rows('session')]) + return account_scan_merged(store, transport(runner)) + + def test_scan_merged_inserts_once(tmp_path: Path) -> None: store = Store(tmp_path) store.write("session", "insert", "s1", {"id": "s1", "kind": "human", "status": "active"}) From a95f62cb9505bd5988bfd903c39a79cc2f222791 Mon Sep 17 00:00:00 2001 From: Jonny Luca <320529100+JonnyLuca@users.noreply.github.com> Date: Sun, 6 Sep 2026 20:36:26 +0000 Subject: [PATCH 2/8] Close credential bypasses and preserve container repository context. --- AGENTS.md | 3 +- DESIGN.md | 27 ++- README.md | 9 +- docs/github-accounts.md | 47 +++- src/agent_cli/git_act.py | 56 ++++- src/agent_cli/github_accounts.py | 332 ++++++++++++++++++++++++++- src/agent_cli/main.py | 25 +- tests/test_git_act.py | 183 ++++++++++++++- tests/test_github_accounts.py | 382 ++++++++++++++++++++++++++++++- tests/test_run.py | 2 +- 10 files changed, 1002 insertions(+), 64 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 2ff976a..6ef4e41 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -18,7 +18,8 @@ monitoring and informs a model when an observed event provides useful work. Installation defaults for GitHub accounts, AI accounts, roles, and selections are unconfigured (`NULL`). Add them explicitly through configuration, with no fixed count. See DESIGN.md §19.8 and docs/github-accounts.md for the implemented -GitHub configuration and remaining AI/role configuration gap. +GitHub configuration, remaining ambient-`gh` gaps (such as `agent a38` +visibility lookup), and the AI/role configuration gap. Draft publication is immediate after the first signed task commit; see the lifecycle. A draft plus local tests is not done. Ready for review is signed diff --git a/DESIGN.md b/DESIGN.md index 57ad7e5..7abcc46 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -650,17 +650,22 @@ limit on their number. A missing setting is unconfigured, not permission to select a built-in identity, provider account, or role. The device-local [GitHub account configuration](docs/github-accounts.md) implements -explicit GitHub accounts and session bindings. An absent or empty configuration -does not authorize GitHub execution. The executor verifies the selected login -and never falls back to an ambient login or a different configured account. -GitHub execution identities do not change the device's hub identity or transfer -ownership of store rows. - -**Remaining implementation boundary:** this GitHub configuration does not yet -implement configurable AI accounts or user-defined roles. The role/vendor lists -and model choices in `lane.py`, and the Grok default in `runtime.py`, still -contain fixed values. They must not be presented as satisfying the complete -empty-default configuration requirement. +explicit GitHub accounts and session bindings for the static executors that load +it: `agent github pending`, assignment and PR-merge scans, supervised issue +reads, and the `pushed` / `mergeable` steps of `agent run`. An absent or empty +configuration does not authorize those covered paths. On those paths the +executor verifies the selected login and never falls back to an ambient login +or a different configured account. GitHub execution identities do not change the +device's hub identity or transfer ownership of store rows. + +**Remaining implementation boundary:** this GitHub configuration does not cover +every CLI path that may invoke `gh`. In particular, `agent a38` visibility +lookup still runs ambient host `gh repo view` when `--private` is omitted +(`src/agent_cli/a38.py`), without loading `github-accounts.json` or a session +binding. This manifest also does not yet implement configurable AI accounts or +user-defined roles. The role/vendor lists and model choices in `lane.py`, and +the Grok default in `runtime.py`, still contain fixed values. They must not be +presented as satisfying the complete empty-default configuration requirement. ## 20. Refused: hub as a coding control plane diff --git a/README.md b/README.md index bfb3e8d..ac896c4 100644 --- a/README.md +++ b/README.md @@ -24,9 +24,12 @@ model when an observed event provides useful work; see DESIGN.md §19.1. Accounts and roles must start unconfigured (`NULL`); see [DESIGN.md §19.8](DESIGN.md#198-configuration-starts-empty). Configure GitHub execution accounts and session bindings explicitly in -[`github-accounts.json`](docs/github-accounts.md). There is no default GitHub -account or fallback to the host login. AI-account and role configuration remain -separate implementation work, as recorded in the design. +[`github-accounts.json`](docs/github-accounts.md) for the executors that load +it. There is no default GitHub account or fallback to the host login on those +paths. Some legacy CLI paths still use ambient host `gh` (for example +`agent a38` visibility lookup when `--private` is omitted); that gap is named +in the design and the accounts document. AI-account and role configuration +remain separate implementation work, as recorded in the design. ## Install diff --git a/docs/github-accounts.md b/docs/github-accounts.md index 3a47e23..3a06c5e 100644 --- a/docs/github-accounts.md +++ b/docs/github-accounts.md @@ -1,9 +1,11 @@ # GitHub execution accounts -Static scripts select GitHub accounts from `$AGENT_HOME/github-accounts.json`. -Installation creates no accounts or bindings. A missing file, `{}`, or null/empty -`accounts` and `sessions` leaves GitHub execution unconfigured. There is no -implicit account from hub pairing, the host login, or environment tokens. +Static scripts that load this manifest select GitHub accounts from +`$AGENT_HOME/github-accounts.json`. Installation creates no accounts or +bindings. A missing file, `{}`, or null/empty `accounts` and `sessions` leaves +those covered GitHub executors unconfigured. There is no implicit account from +hub pairing, the host login, or environment tokens. CLI paths that never load +this file are outside this enforcement; see the remaining gaps below. The following is an operator-supplied example, not an installed default: @@ -55,8 +57,20 @@ GitHub token variables for the child process, does not change the process-wide environment, and does not switch the active account in another configuration directory. Authentication failure or a mismatch blocks the action; there is no fallback. Git operations use the selected account's credential helper and -signing configuration. Use HTTPS GitHub remotes; SSH transport and interactive -credential prompts are disabled for this account runner. +signing configuration. Before supported `fetch` / `push` / `pull` / `clone` +forms, the runner asks Git for the effective remote URL via +`git remote get-url [--push] --all` (which already applies a distinct `pushurl` +and `insteadOf` / `pushInsteadOf` rewrite effects) and rejects +credential-bearing, non-HTTPS, or non-`github.com` network remotes. Explicit +URL arguments are resolved the same way through a temporary command-scoped +remote. Only transfer forms with one explicit repository argument are accepted +(for example `git fetch -- origin`, `git push -- origin HEAD:refs/heads/feature`, +and `git push --set-upstream origin feature`); implicit default-remote forms, +`fetch --all` / `--multiple`, and `--repo` combined with a different positional +repository are refused. Rejection messages never echo URL values. Local Git +metadata commands are unchanged and are not treated as network transfers. Use +HTTPS GitHub remotes; SSH transport and interactive credential prompts are +disabled for this account runner. `agent github pending` binds each GitHub activity to its session's configured account. `execution_account` records the account name and expected login. A @@ -65,13 +79,22 @@ its author to match; a PR authored by another account is not silently adopted. The same session selection applies to assignment scans, PR-merge scans, supervised issue reads, and the `pushed`/`mergeable` steps of `agent run`. -Account selection is script configuration, never an instruction taken from an -issue body or a model's activity payload. Configure existing sessions explicitly -before enabling these operations after upgrading. +The `mergeable` step passes an explicit `--repo` together with a PR number or +branch selector: when the task already records the pull-request target +(`repo` + numeric `ref`), that pair is used (fork targets may differ from +origin) and Git signing identity is not required; otherwise the branch comes +from mapped `git -C` and the repo from the validated origin remote. Either path +keeps container-backed accounts independent of the executor's default working +directory. Account selection is script configuration, never an instruction +taken from an issue body or a model's activity payload. Configure existing +sessions explicitly before enabling these operations after upgrading. Hub pairing and event ownership are separate: using a second GitHub execution account does not re-pair the device or change ownership of its rows. -This document covers GitHub execution. Configurable AI accounts and roles are -part of the [empty-default requirement](../DESIGN.md#198-configuration-starts-empty); -they are not implemented by this manifest. +**Remaining gaps.** This manifest does not cover legacy ambient `gh` paths that +never load it. One reachable example is `agent a38` visibility lookup +(`gh repo view` when `--private` is omitted), which still uses the host `gh` +login. Configurable AI accounts and roles are part of the +[empty-default requirement](../DESIGN.md#198-configuration-starts-empty) and are +not implemented by this manifest. diff --git a/src/agent_cli/git_act.py b/src/agent_cli/git_act.py index 9214d49..b163623 100644 --- a/src/agent_cli/git_act.py +++ b/src/agent_cli/git_act.py @@ -13,6 +13,7 @@ PROTECTED = frozenset({"develop", "main", "master"}) _SHA_RE = re.compile(r"^[0-9a-fA-F]{7,40}$") +_REPO_NAME = re.compile(r"^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$") class GitActError(Exception): @@ -105,13 +106,52 @@ def push_branch(*, cwd: str, runner: Runner) -> str: def measure_mergeable( - *, cwd: str, runner: Runner, expected_head: str | None = None + *, + cwd: str, + runner: Runner, + expected_head: str | None = None, + repo: str | None = None, + number: int | None = None, ) -> str: """Return a short evidence string when the current branch PR is MERGEABLE - and every GitHub check is SUCCESS (or there are no checks). Else raise GitActError.""" - _ = cwd # gh argv has no -C; cwd is inherited from _exec_argv + and every GitHub check is SUCCESS (or there are no checks). Else raise GitActError. + + Prefer an explicit task ``repo`` + PR ``number`` (fork target may differ from + origin). Otherwise derive the branch via mapped ``git -C`` and a validated + origin remote. Every ``gh`` call gets both ``--repo`` and a PR selector so + container-backed accounts do not depend on executor cwd. + """ + from .github_accounts import GitHubHttpsRemoteError, validate_repo_remote + + selector: str + target_repo: str + if repo is not None or number is not None: + if repo is None or number is None: + raise GitActError("pull request repo and number must be provided together") + if not isinstance(repo, str) or _REPO_NAME.fullmatch(repo) is None: + raise GitActError("invalid pull request repository") + if isinstance(number, bool) or not isinstance(number, int) or number <= 0: + raise GitActError("invalid pull request number") + target_repo = repo + selector = str(number) + else: + try: + target_repo = validate_repo_remote(runner, cwd, "origin") + except GitHubHttpsRemoteError as exc: + raise GitActError(str(exc)) from exc + completed = runner(_git(cwd, "rev-parse", "--abbrev-ref", "HEAD")) + if completed.returncode != 0: + raise GitActError(_fail_detail(completed, "git failed")) + branch = completed.stdout.strip() + if not branch or branch == "HEAD": + raise GitActError("empty branch name") + selector = branch + completed = runner( - ["gh", "pr", "view", "--json", "mergeable,state,url,number,headRefOid"] + [ + "gh", "pr", "view", selector, "--repo", target_repo, + "--json", "mergeable,state,url,number,headRefOid", + ] ) if completed.returncode != 0: raise GitActError(_fail_detail(completed, "gh failed")) @@ -129,8 +169,8 @@ def measure_mergeable( state_ok = isinstance(state, str) and state.upper() == "OPEN" if mergeable != "MERGEABLE" or not state_ok: raise GitActError(f"mergeable={mergeable!r} state={state!r}") - number = view.get("number") - if isinstance(number, bool) or not isinstance(number, int) or number <= 0: + view_number = view.get("number") + if isinstance(view_number, bool) or not isinstance(view_number, int) or view_number <= 0: raise GitActError("pr view missing number") oid = view.get("headRefOid") if not isinstance(oid, str) or oid == "": @@ -142,7 +182,7 @@ def measure_mergeable( raise GitActError(f"pr head {oid} does not match {want}") completed = runner( - ["gh", "pr", "checks", str(number), "--json", "name,state"] + ["gh", "pr", "checks", str(view_number), "--repo", target_repo, "--json", "name,state"] ) if completed.returncode != 0: raise GitActError(_fail_detail(completed, "gh failed")) @@ -165,4 +205,4 @@ def measure_mergeable( if str(check_state or "").upper() != "SUCCESS": raise GitActError(f"check {name} is {check_state}") - return f"mergeable number={number} checks=ok" + return f"mergeable number={view_number} checks=ok" diff --git a/src/agent_cli/github_accounts.py b/src/agent_cli/github_accounts.py index 145b19c..a52e12b 100644 --- a/src/agent_cli/github_accounts.py +++ b/src/agent_cli/github_accounts.py @@ -1,17 +1,21 @@ """Device-local GitHub account bindings for static executors. -No configured account means no GitHub execution. Credentials stay in each -account's gh configuration directory, never in activities or this manifest. +Covered executors load this manifest and refuse to run when it is absent or +empty. Credentials stay in each account's gh configuration directory, never in +activities or this manifest. Legacy ambient `gh` paths that do not load this +file are outside this module's enforcement; see docs/github-accounts.md. """ from __future__ import annotations import json import re +import secrets from collections.abc import Callable from dataclasses import dataclass from pathlib import Path from typing import Any +from urllib.parse import urlparse from .runtime import Completed from .store import StoreError @@ -19,22 +23,333 @@ Runner = Callable[[list[str]], Completed] CONFIG_FILE = "github-accounts.json" _LOGIN = re.compile(r"[A-Za-z0-9](?:[A-Za-z0-9-]*[A-Za-z0-9])?") +_OWNER_NAME = re.compile(r"^[A-Za-z0-9_.-]+$") _TOKEN_ENV = ( "GH_TOKEN", "GITHUB_TOKEN", "GH_ENTERPRISE_TOKEN", "GITHUB_ENTERPRISE_TOKEN", "GIT_ASKPASS", "SSH_ASKPASS", ) +_NETWORK_GIT = frozenset({"fetch", "push", "pull", "clone"}) +_GIT_OPTS_WITH_ARG = frozenset({ + "-C", "-c", "-o", + "--git-dir", "--work-tree", "--namespace", "--super-prefix", "--config-env", + "--buffered-output-size", +}) +_FETCH_PUSH_OPTS_WITH_ARG = frozenset({ + "-o", "--upload-pack", "--exec", "--depth", "--shallow-since", "--shallow-exclude", + "--deepen", "--negotiation-tip", "--jobs", "--server-option", "--recv-pack", + "--push-option", "--repo", +}) class AccountError(StoreError): """Configuration or authentication does not authorize this execution.""" +class GitHubHttpsRemoteError(AccountError): + """A Git remote is not a safe HTTPS github.com URL. + + Messages must never include remote URL values (they may embed credentials). + """ + + def _text(value: Any, field: str) -> str: if not isinstance(value, str) or not value.strip() or "\x00" in value or "\n" in value: raise AccountError(f"{field} must be a non-empty single-line string") return value +def ensure_github_https_remote(url: str) -> str: + """Return ``owner/name`` for a credential-free HTTPS github.com remote. + + Rejects SSH, non-GitHub hosts, non-HTTPS schemes, embedded userinfo, and + malformed paths. Error text never includes the URL value. + """ + if not isinstance(url, str) or not url.strip() or "\x00" in url or "\n" in url or "\r" in url: + raise GitHubHttpsRemoteError("remote URL is unsafe") + text = url.strip() + parsed = urlparse(text) + if parsed.scheme != "https": + raise GitHubHttpsRemoteError("remote must be HTTPS GitHub") + if parsed.username is not None or parsed.password is not None: + raise GitHubHttpsRemoteError("remote URL must not contain credentials") + if "@" in (parsed.netloc or ""): + # urlparse missed userinfo; still refuse without echoing the value. + raise GitHubHttpsRemoteError("remote URL must not contain credentials") + try: + host = (parsed.hostname or "").casefold() + port = parsed.port + except ValueError: + # Malformed ports and some bad netlocs raise ValueError; never echo URL. + raise GitHubHttpsRemoteError("remote URL is unsafe") from None + if host != "github.com" or port not in {None, 443}: + raise GitHubHttpsRemoteError("remote must be HTTPS GitHub") + if parsed.query or parsed.fragment: + raise GitHubHttpsRemoteError("remote must be HTTPS GitHub owner/name") + path = parsed.path.strip("/") + if path.endswith(".git"): + path = path[:-4] + parts = path.split("/") + if len(parts) != 2 or not parts[0] or not parts[1]: + raise GitHubHttpsRemoteError("remote must be HTTPS GitHub owner/name") + owner, name = parts + if _OWNER_NAME.fullmatch(owner) is None or _OWNER_NAME.fullmatch(name) is None: + raise GitHubHttpsRemoteError("remote must be HTTPS GitHub owner/name") + return f"{owner}/{name}" + + +def _looks_like_refspec(value: str) -> bool: + if value.startswith("+") or value == "HEAD" or value.startswith("refs/"): + return True + if "://" in value or value.startswith("git@"): + return False + if ":" in value: + source, _, dest = value.partition(":") + if source in {"", "HEAD"} or source.startswith("refs/") or dest.startswith("refs/"): + return True + return False + + +def _looks_like_url(value: str) -> bool: + if "://" in value or value.startswith("git@") or value.startswith("file:"): + return True + # scp-like host:path — not a Git refspec and not an absolute path + if ":" in value and not value.startswith("/") and not _looks_like_refspec(value): + host, _, rest = value.partition(":") + if host and "/" not in host and rest and not rest.startswith(":"): + return True + return False + + +def _git_cwd_argv(cwd: str | None, *parts: str) -> list[str]: + if cwd is None: + return ["git", *parts] + return ["git", "-C", cwd, *parts] + + +def _temp_remote_name() -> str: + return f"_agent_url_{secrets.token_hex(8)}" + + +def _parse_get_url_stdout(stdout: str) -> list[str]: + urls = [line.strip() for line in stdout.splitlines() if line.strip()] + if not urls: + raise GitHubHttpsRemoteError("remote URL is not configured") + return urls + + +def _remote_get_urls(run: Runner, cwd: str | None, remote: str, *, push: bool) -> list[str]: + """Return effective URLs from ``git remote get-url`` (rewrites already applied).""" + if not isinstance(remote, str) or not remote.strip() or "\x00" in remote or "\n" in remote or "\r" in remote: + raise GitHubHttpsRemoteError("remote name is unsafe") + name = remote.strip() + if _looks_like_url(name): + raise GitHubHttpsRemoteError("remote name is unsafe") + argv = _git_cwd_argv(cwd, "remote", "get-url") + if push: + argv.append("--push") + argv.extend(["--all", "--", name]) + completed = run(argv) + if completed.returncode != 0: + # Distinct push URL absent: fall back to fetch URLs (Git's usual behavior). + if push: + return _remote_get_urls(run, cwd, name, push=False) + raise GitHubHttpsRemoteError("remote URL is not configured") + return _parse_get_url_stdout(completed.stdout) + + +def _explicit_url_get_urls(run: Runner, cwd: str | None, url: str, *, push: bool) -> list[str]: + """Resolve rewrite effects for an explicit URL via a command-scoped temporary remote.""" + if not isinstance(url, str) or not url.strip() or "\x00" in url or "\n" in url or "\r" in url: + raise GitHubHttpsRemoteError("remote URL is unsafe") + text = url.strip() + token = _temp_remote_name() + # Never print text; pass it only as a command-scoped git config value. + argv = _git_cwd_argv(cwd, "-c", f"remote.{token}.url={text}", "remote", "get-url") + if push: + argv.append("--push") + argv.extend(["--all", "--", token]) + completed = run(argv) + if completed.returncode != 0: + if push: + return _explicit_url_get_urls(run, cwd, text, push=False) + raise GitHubHttpsRemoteError("remote URL is not configured") + return _parse_get_url_stdout(completed.stdout) + + +def resolve_effective_github_https_url( + run: Runner, + cwd: str | None, + remote: str, + *, + push: bool = False, +) -> str: + """Resolve one effective remote URL after pushurl and rewrite effects. + + Relies on ``git remote get-url [--push] --all``, which already applies + ``insteadOf`` / ``pushInsteadOf``. Explicit URLs are resolved through a + temporary command-scoped remote so the same Git rewrite path runs. Every + resulting URL is validated; the first accepted URL string is returned. + Callers that need ``owner/name`` should use ``validate_repo_remote``. + """ + if _looks_like_url(remote): + urls = _explicit_url_get_urls(run, cwd, remote, push=push) + else: + urls = _remote_get_urls(run, cwd, remote, push=push) + for raw in urls: + ensure_github_https_remote(raw) + return urls[0] + + +def validate_repo_remote(run: Runner, cwd: str | None, remote: str) -> str: + """Validate fetch and push effective URLs for ``remote``; return ``owner/name``.""" + fetch_url = resolve_effective_github_https_url(run, cwd, remote, push=False) + push_url = resolve_effective_github_https_url(run, cwd, remote, push=True) + fetch_repo = ensure_github_https_remote(fetch_url) + push_repo = ensure_github_https_remote(push_url) + if fetch_repo.casefold() != push_repo.casefold(): + raise GitHubHttpsRemoteError("fetch and push remotes resolve to different GitHub repositories") + return fetch_repo + + +def _skip_git_option(args: list[str], index: int) -> int: + arg = args[index] + name = arg.split("=", 1)[0] + if arg.startswith("--") and "=" in arg: + return index + 1 + if arg in _GIT_OPTS_WITH_ARG or name in _GIT_OPTS_WITH_ARG: + return index + 2 + return index + 1 + + +def _split_git_command(git_args: list[str]) -> tuple[str | None, list[str]]: + index = 0 + while index < len(git_args): + arg = git_args[index] + if arg == "--": + index += 1 + break + if arg.startswith("-"): + index = _skip_git_option(git_args, index) + continue + return arg, git_args[index + 1 :] + if index < len(git_args): + return git_args[index], git_args[index + 1 :] + return None, [] + + +def _positionals(rest: list[str], *, opts_with_arg: frozenset[str]) -> list[str]: + if "--" in rest: + return [item for item in rest[rest.index("--") + 1 :] if item] + index = 0 + out: list[str] = [] + while index < len(rest): + arg = rest[index] + if arg.startswith("-"): + name = arg.split("=", 1)[0] + if arg.startswith("--") and "=" in arg: + index += 1 + continue + if arg in opts_with_arg or name in opts_with_arg: + index += 2 + continue + index += 1 + continue + out.append(arg) + index += 1 + return out + + +def _git_c_path(git_args: list[str]) -> str | None: + if "-C" not in git_args: + return None + index = git_args.index("-C") + 1 + if index >= len(git_args): + raise AccountError("git -C requires a worktree path") + return git_args[index] + + +def _args_before_double_dash(rest: list[str]) -> list[str]: + if "--" in rest: + return rest[: rest.index("--")] + return rest + + +def _has_flag(rest: list[str], name: str) -> bool: + for arg in _args_before_double_dash(rest): + if arg == name or arg.startswith(name + "="): + return True + return False + + +def _option_value(rest: list[str], name: str) -> str | None: + args = _args_before_double_dash(rest) + index = 0 + while index < len(args): + arg = args[index] + if arg.startswith(name + "="): + return arg.split("=", 1)[1] + if arg == name: + if index + 1 >= len(args): + raise GitHubHttpsRemoteError("unsupported git network command form") + return args[index + 1] + index += 1 + return None + + +def _network_remote_targets(git_args: list[str]) -> list[str] | None: + """Return the single explicit remote/URL to validate, or None if not network. + + Supported transfer forms (explicit single repository argument): + - ``git fetch -- origin`` / ``git fetch origin [...]`` + - ``git push -- origin HEAD:refs/heads/feature`` + - ``git push --set-upstream origin feature`` / ``git push -u origin feature`` + - ``git pull`` with an explicit remote + - ``git clone `` + + Implicit default-remote forms, ``fetch --all`` / ``--multiple``, and + ``--repo`` combined with a different positional repository are rejected. + Local metadata commands are not treated as network transfers. + """ + verb, rest = _split_git_command(git_args) + if verb is None or verb not in _NETWORK_GIT: + return None + if verb == "clone": + positionals = _positionals(rest, opts_with_arg=_FETCH_PUSH_OPTS_WITH_ARG | _GIT_OPTS_WITH_ARG) + if not positionals: + raise GitHubHttpsRemoteError("git clone requires a repository URL") + return [positionals[0]] + if verb == "fetch" and (_has_flag(rest, "--all") or _has_flag(rest, "--multiple")): + raise GitHubHttpsRemoteError("unsupported git fetch form") + if verb == "pull" and (_has_flag(rest, "--all") or _has_flag(rest, "--multiple")): + raise GitHubHttpsRemoteError("unsupported git pull form") + repo_opt = _option_value(rest, "--repo") + positionals = _positionals(rest, opts_with_arg=_FETCH_PUSH_OPTS_WITH_ARG) + if repo_opt is not None: + if positionals: + raise GitHubHttpsRemoteError("unsupported git network command form") + if not repo_opt.strip(): + raise GitHubHttpsRemoteError("unsupported git network command form") + return [repo_opt] + if not positionals: + raise GitHubHttpsRemoteError(f"git {verb} requires an explicit remote") + first = positionals[0] + # Refspec without a repository uses branch.remote / remote.pushDefault — unsupported. + if _looks_like_refspec(first) and not _looks_like_url(first): + raise GitHubHttpsRemoteError(f"git {verb} requires an explicit remote") + return [first] + + +def _map_worktree_path(path: Path, mappings: tuple[tuple[str, str], ...]) -> str: + if not path.is_absolute() or ".." in path.parts: + raise AccountError("Git worktree path must be absolute without parent traversal") + mapped = str(path) + for source, target in sorted(mappings, key=lambda pair: len(pair[0]), reverse=True): + if path.is_relative_to(source): + mapped = str(Path(target) / path.relative_to(source)) + break + return mapped + + @dataclass(frozen=True) class Account: name: str @@ -75,13 +390,12 @@ def scoped(argv: list[str]) -> Completed: index = git_args.index("-C") + 1 if index >= len(git_args): raise AccountError("git -C requires a worktree path") - path = Path(git_args[index]) - if not path.is_absolute() or ".." in path.parts: - raise AccountError("Git worktree path must be absolute without parent traversal") - for source, target in sorted(self.worktree_paths, key=lambda pair: len(pair[0]), reverse=True): - if path.is_relative_to(source): - git_args[index] = str(Path(target) / path.relative_to(source)) - break + git_args[index] = _map_worktree_path(Path(git_args[index]), self.worktree_paths) + targets = _network_remote_targets(git_args) + if targets is not None: + cwd = _git_c_path(git_args) + for target in targets: + validate_repo_remote(scoped, cwd, target) command = [ "git", "-c", "core.askPass=", "-c", "http.extraHeader=", "-c", "http.https://github.com/.extraHeader=", "-c", "credential.helper=", diff --git a/src/agent_cli/main.py b/src/agent_cli/main.py index 9391196..d940bff 100644 --- a/src/agent_cli/main.py +++ b/src/agent_cli/main.py @@ -2668,11 +2668,26 @@ def cmd_run(args: list[str]) -> None: try: expected = str(snap.get("head_sha") or head or "").strip() or None account = load_accounts(store.home).for_session(str(snap.get("session_id") or "")) - evidence = measure_mergeable( - cwd=cwd, - runner=account.runner(lambda argv: _exec_argv(argv, cwd=cwd)), - expected_head=expected, - ) + pull_request = _task_pull_request(_need(store, "task", tid)) + if pull_request is not None: + pr_repo, pr_number = pull_request + evidence = measure_mergeable( + cwd=cwd, + runner=account.runner( + lambda argv: _exec_argv(argv, cwd=cwd), require_git=False + ), + expected_head=expected, + repo=pr_repo, + number=pr_number, + ) + else: + evidence = measure_mergeable( + cwd=cwd, + runner=account.runner( + lambda argv: _exec_argv(argv, cwd=cwd), require_git=True + ), + expected_head=expected, + ) except GitActError as exc: die(str(exc)) if step.key in NO_AUTO_CLOSE: diff --git a/tests/test_git_act.py b/tests/test_git_act.py index d8c51cf..393779d 100644 --- a/tests/test_git_act.py +++ b/tests/test_git_act.py @@ -13,6 +13,9 @@ CWD = "/tmp/repo" SHA = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" +ORIGIN = "https://github.com/owner/repo.git" +REPO = "owner/repo" +BRANCH = "feat-x" FORCE_FLAGS = ("--force", "--force-with-lease", "-f") PUSH_ARGV = ["git", "-C", CWD, "push", "--", "origin", "HEAD:refs/heads/feat-x"] @@ -28,12 +31,47 @@ def _config(argv: list[str]) -> Completed | None: return Completed(1, "", "") +def _apply_rules(url: str, rules: list[tuple[str, str]]) -> str: + best: tuple[str, str] | None = None + for base, old in rules: + if url.startswith(old) and (best is None or len(old) > len(best[1])): + best = (base, old) + if best is None: + return url + return best[0] + url[len(best[1]) :] + + +def _origin_resolution( + argv: list[str], + *, + url: str = ORIGIN, + instead_of: list[tuple[str, str]] | None = None, + push_instead_of: list[tuple[str, str]] | None = None, +) -> Completed | None: + """Simulate git remote get-url (rewrites already applied, as real Git does).""" + if argv[:3] != ["git", "-C", CWD]: + return None + if argv == ["git", "-C", CWD, "rev-parse", "--abbrev-ref", "HEAD"]: + return Completed(0, BRANCH + "\n", "") + if "remote" in argv and "get-url" in argv: + effective = url + effective = _apply_rules(effective, instead_of or []) + if "--push" in argv: + effective = _apply_rules(effective, push_instead_of or []) + return Completed(0, effective + "\n", "") + return None + + def _assert_git_c(argv: list[str]) -> None: assert argv[:3] == ["git", "-C", CWD] for flag in FORCE_FLAGS: assert flag not in argv +def _mergeable_view_argv(selector: str = BRANCH, repo: str = REPO) -> list[str]: + return ["gh", "pr", "view", selector, "--repo", repo] + + def test_push_ahead_one_pushes_without_force() -> None: calls: list[list[str]] = [] @@ -221,7 +259,11 @@ def runner(argv: list[str]) -> Completed: def test_mergeable_open_empty_checks() -> None: def runner(argv: list[str]) -> Completed: - if "pr" in argv and "view" in argv: + origin = _origin_resolution(argv) + if origin is not None: + return origin + if argv[:5] == _mergeable_view_argv(): + assert argv[argv.index("--repo") + 1] == REPO return Completed( 0, json.dumps( @@ -235,7 +277,8 @@ def runner(argv: list[str]) -> Completed: ), "", ) - if "checks" in argv: + if argv[:4] == ["gh", "pr", "checks", "1"] and "--repo" in argv: + assert argv[argv.index("--repo") + 1] == REPO return Completed(0, "[]", "") raise AssertionError(f"unexpected argv: {argv}") @@ -247,7 +290,10 @@ def runner(argv: list[str]) -> Completed: def test_mergeable_all_success() -> None: def runner(argv: list[str]) -> Completed: - if "pr" in argv and "view" in argv: + origin = _origin_resolution(argv) + if origin is not None: + return origin + if argv[:5] == _mergeable_view_argv(): return Completed( 0, json.dumps( @@ -262,6 +308,7 @@ def runner(argv: list[str]) -> Completed: "", ) if "checks" in argv: + assert "--repo" in argv and argv[argv.index("--repo") + 1] == REPO return Completed( 0, json.dumps( @@ -280,7 +327,10 @@ def runner(argv: list[str]) -> Completed: def test_mergeable_conflicting() -> None: def runner(argv: list[str]) -> Completed: - if "pr" in argv and "view" in argv: + origin = _origin_resolution(argv) + if origin is not None: + return origin + if argv[:5] == _mergeable_view_argv(): return Completed( 0, json.dumps( @@ -302,7 +352,10 @@ def runner(argv: list[str]) -> Completed: def test_mergeable_check_failure() -> None: def runner(argv: list[str]) -> Completed: - if "pr" in argv and "view" in argv: + origin = _origin_resolution(argv) + if origin is not None: + return origin + if argv[:5] == _mergeable_view_argv(): return Completed( 0, json.dumps( @@ -330,7 +383,10 @@ def runner(argv: list[str]) -> Completed: def test_mergeable_check_pending() -> None: def runner(argv: list[str]) -> Completed: - if "pr" in argv and "view" in argv: + origin = _origin_resolution(argv) + if origin is not None: + return origin + if argv[:5] == _mergeable_view_argv(): return Completed( 0, json.dumps( @@ -358,7 +414,10 @@ def runner(argv: list[str]) -> Completed: def test_mergeable_check_skipped() -> None: def runner(argv: list[str]) -> Completed: - if "pr" in argv and "view" in argv: + origin = _origin_resolution(argv) + if origin is not None: + return origin + if argv[:5] == _mergeable_view_argv(): return Completed( 0, json.dumps( @@ -386,7 +445,10 @@ def runner(argv: list[str]) -> Completed: def test_mergeable_head_mismatch() -> None: def runner(argv: list[str]) -> Completed: - if "pr" in argv and "view" in argv: + origin = _origin_resolution(argv) + if origin is not None: + return origin + if argv[:5] == _mergeable_view_argv(): return Completed( 0, json.dumps( @@ -408,7 +470,10 @@ def runner(argv: list[str]) -> Completed: def test_mergeable_missing_number() -> None: def runner(argv: list[str]) -> Completed: - if "pr" in argv and "view" in argv: + origin = _origin_resolution(argv) + if origin is not None: + return origin + if argv[:5] == _mergeable_view_argv(): return Completed( 0, json.dumps( @@ -424,3 +489,103 @@ def runner(argv: list[str]) -> Completed: with pytest.raises(GitActError, match="missing number"): measure_mergeable(cwd=CWD, runner=runner) + + +def test_mergeable_rejects_credential_origin_without_leaking() -> None: + secret = "super-secret-token" + + def runner(argv: list[str]) -> Completed: + origin = _origin_resolution( + argv, url=f"https://x-access-token:{secret}@github.com/owner/repo.git" + ) + if origin is not None: + return origin + raise AssertionError("gh must not run when origin is unsafe") + + with pytest.raises(GitActError, match="must not contain credentials") as excinfo: + measure_mergeable(cwd=CWD, runner=runner) + assert secret not in str(excinfo.value) + + +def test_mergeable_passes_explicit_repo_and_branch_for_container_context() -> None: + """Regression: gh must receive --repo and a PR selector so docker-exec needs no cwd.""" + gh_calls: list[list[str]] = [] + + def runner(argv: list[str]) -> Completed: + origin = _origin_resolution(argv) + if origin is not None: + return origin + if argv[0] == "gh": + gh_calls.append(list(argv)) + if "view" in argv: + assert argv[1:4] == ["pr", "view", BRANCH] + assert "--repo" in argv and argv[argv.index("--repo") + 1] == REPO + return Completed( + 0, + json.dumps( + { + "mergeable": "MERGEABLE", + "state": "OPEN", + "url": "https://example.invalid/p/9", + "number": 9, + "headRefOid": SHA, + } + ), + "", + ) + if "checks" in argv: + assert "--repo" in argv and argv[argv.index("--repo") + 1] == REPO + return Completed(0, "[]", "") + raise AssertionError(f"unexpected argv: {argv}") + + evidence = measure_mergeable(cwd=CWD, runner=runner) + assert "number=9" in evidence + assert gh_calls + for call in gh_calls: + assert "--repo" in call + assert call[call.index("--repo") + 1] == REPO + assert "-C" not in call + assert "--workdir" not in call + + +def test_mergeable_uses_explicit_task_pr_without_git() -> None: + """Known task PR target (fork upstream) must not consult origin or cwd.""" + fork_target = "upstream/product" + gh_calls: list[list[str]] = [] + + def runner(argv: list[str]) -> Completed: + if argv and argv[0] == "git": + raise AssertionError("explicit repo+number must not call git") + if argv[0] == "gh": + gh_calls.append(list(argv)) + if "view" in argv: + assert argv[:6] == ["gh", "pr", "view", "42", "--repo", fork_target] + return Completed( + 0, + json.dumps( + { + "mergeable": "MERGEABLE", + "state": "OPEN", + "url": "https://example.invalid/p/42", + "number": 42, + "headRefOid": SHA, + } + ), + "", + ) + if "checks" in argv: + assert argv[:4] == ["gh", "pr", "checks", "42"] + assert argv[argv.index("--repo") + 1] == fork_target + return Completed(0, "[]", "") + raise AssertionError(f"unexpected argv: {argv}") + + evidence = measure_mergeable( + cwd="/nonexistent/executor/cwd", + runner=runner, + repo=fork_target, + number=42, + ) + assert "number=42" in evidence + assert gh_calls + for call in gh_calls: + assert call[call.index("--repo") + 1] == fork_target diff --git a/tests/test_github_accounts.py b/tests/test_github_accounts.py index 23b2f0e..fcf972f 100644 --- a/tests/test_github_accounts.py +++ b/tests/test_github_accounts.py @@ -7,12 +7,98 @@ import pytest -from agent_cli.github_accounts import Account, AccountError, load_accounts +from agent_cli.git_act import measure_mergeable +from agent_cli.github_accounts import ( + Account, + AccountError, + GitHubHttpsRemoteError, + ensure_github_https_remote, + load_accounts, + resolve_effective_github_https_url, + validate_repo_remote, +) from agent_cli.github_act import scan_github from agent_cli.runtime import Completed, run_argv from agent_cli.store import Store from agent_cli.watch import scan_assigned, scan_merged +IDENTITY = { + 'name': 'Worker One', + 'email': 'one@example.com', + 'signing_key': '/keys/one', + 'signing_format': 'ssh', +} +SAFE_ORIGIN = 'https://github.com/owner/repo.git' + + +def _git_payload(argv: list[str]) -> list[str]: + """Strip account runner env/prefix wrappers down to the git argv.""" + if 'git' not in argv: + return argv + return argv[argv.index('git'):] + + +def _apply_url_rules(url: str, rules: list[tuple[str, str]]) -> str: + best: tuple[str, str] | None = None + for base, old in rules: + if url.startswith(old) and (best is None or len(old) > len(best[1])): + best = (base, old) + if best is None: + return url + return best[0] + url[len(best[1]):] + + +def _remote_script( + *, + fetch_url: str = SAFE_ORIGIN, + push_url: str | None = None, + instead_of: list[tuple[str, str]] | None = None, + push_instead_of: list[tuple[str, str]] | None = None, +): + """Fake git remote get-url the way real Git does: rewrites already applied.""" + push_url = fetch_url if push_url is None else push_url + instead_of = instead_of or [] + push_instead_of = push_instead_of or [] + + def handle(argv: list[str]) -> Completed | None: + git = _git_payload(argv) + if not git or git[0] != 'git': + return None + if 'remote' not in git or 'get-url' not in git: + return None + explicit_url = None + explicit_name = None + index = 1 + while index < len(git): + arg = git[index] + if arg == '-c' and index + 1 < len(git): + cfg = git[index + 1] + if cfg.startswith('remote.') and '.url=' in cfg: + key, _, value = cfg.partition('=') + parts = key.split('.') + if len(parts) >= 3 and parts[0] == 'remote' and parts[-1] == 'url': + explicit_name = '.'.join(parts[1:-1]) + explicit_url = value + index += 2 + continue + if arg in {'-C'} and index + 1 < len(git): + index += 2 + continue + index += 1 + name = git[-1] + if explicit_url is not None and name == explicit_name: + url = explicit_url + elif name == 'origin': + url = push_url if '--push' in git else fetch_url + else: + return Completed(1, '', 'unknown remote') + url = _apply_url_rules(url, instead_of) + if '--push' in git: + url = _apply_url_rules(url, push_instead_of) + return Completed(0, url + '\n', '') + + return handle + def configure(home: Path, *, sessions=None) -> None: (home / 'github-accounts.json').write_text(json.dumps({ @@ -159,12 +245,17 @@ def test_git_requires_explicit_identity_and_uses_scoped_helper(): account = Account('one', 'WorkerOne', '/accounts/one') with pytest.raises(AccountError, match='Git identity is not configured'): account.runner(lambda argv: pytest.fail('No auth before missing identity is reported'), require_git=True) - identity = {'name': 'Worker One', 'email': 'one@example.com', 'signing_key': '/keys/one', 'signing_format': 'ssh'} + remotes = _remote_script() calls = [] def runner(argv): calls.append(argv) - return Completed(0, 'WorkerOne', '') - scoped = Account('one', 'WorkerOne', '/accounts/one', identity).runner(runner, require_git=True) + handled = remotes(argv) + if handled is not None: + return handled + if 'gh' in argv: + return Completed(0, 'WorkerOne', '') + return Completed(0, '', '') + scoped = Account('one', 'WorkerOne', '/accounts/one', IDENTITY).runner(runner, require_git=True) scoped(['git', '-C', '/work', 'push', 'origin', 'feature']) command = calls[-1] assert 'GIT_AUTHOR_EMAIL=one@example.com' in command @@ -221,11 +312,17 @@ def test_container_account_keeps_credentials_and_signing_in_its_executor(tmp_pat 'sessions': {'s1': 'container'}, } (tmp_path / 'github-accounts.json').write_text(json.dumps(data)) + remotes = _remote_script() calls = [] def runner(argv): calls.append(argv) assert argv[:5] == ['docker', 'exec', '-i', 'worker-container', 'env'] - return Completed(0, 'ContainerWorker', '') + handled = remotes(argv) + if handled is not None: + return handled + if 'gh' in argv: + return Completed(0, 'ContainerWorker', '') + return Completed(0, '', '') scoped = load_accounts(tmp_path).for_session('s1').runner(runner, require_git=True) scoped(['git', '-C', '/srv/worker/data/repo', 'push', 'origin', 'feature']) command = calls[-1] @@ -238,6 +335,281 @@ def runner(argv): scoped(['git', '-C', '/srv/worker/data/../other', 'status']) +@pytest.mark.no_pg +def test_ensure_github_https_remote_accepts_safe_urls(): + assert ensure_github_https_remote('https://github.com/Owner/Repo.git') == 'Owner/Repo' + assert ensure_github_https_remote('https://github.com/Owner/Repo') == 'Owner/Repo' + assert ensure_github_https_remote('https://github.com:443/Owner/Repo.git') == 'Owner/Repo' + + +@pytest.mark.no_pg +@pytest.mark.parametrize('url', [ + 'https://x-access-token:ghs_secret@github.com/owner/repo.git', + 'https://user:pass@github.com/owner/repo', + 'git@github.com:owner/repo.git', + 'ssh://git@github.com/owner/repo.git', + 'https://gitlab.com/owner/repo.git', + 'https://github.com/owner/repo/extra', + 'http://github.com/owner/repo.git', + '/absolute/local/path', + 'https://github.com:abc/owner/repo.git', + 'https://token:port-secret@github.com:notaport/owner/repo.git', +]) +def test_ensure_github_https_remote_rejects_unsafe_without_leaking(url): + with pytest.raises(GitHubHttpsRemoteError) as excinfo: + ensure_github_https_remote(url) + message = str(excinfo.value) + assert 'ghs_secret' not in message + assert 'pass' not in message + assert 'x-access-token' not in message + assert 'port-secret' not in message + if '://' in url or url.startswith('git@'): + assert url not in message + + +@pytest.mark.no_pg +def test_resolve_effective_url_applies_pushurl_and_rewrites(): + remotes = _remote_script( + fetch_url='https://github.com/owner/repo.git', + push_url='ssh://git@github.com/owner/repo.git', + instead_of=[('https://github.com/', 'ssh://git@github.com/')], + ) + def run(argv): + handled = remotes(argv) + assert handled is not None + return handled + # Fake get-url already applies insteadOf, matching real Git. + assert resolve_effective_github_https_url(run, '/work', 'origin', push=False).startswith('https://') + assert resolve_effective_github_https_url(run, '/work', 'origin', push=True) == 'https://github.com/owner/repo.git' + assert validate_repo_remote(run, '/work', 'origin') == 'owner/repo' + + +@pytest.mark.no_pg +def test_explicit_url_uses_temporary_remote_get_url(): + remotes = _remote_script( + instead_of=[('https://github.com/', 'ssh://git@github.com/')], + ) + seen = [] + def run(argv): + seen.append(list(argv)) + handled = remotes(argv) + assert handled is not None + return handled + url = resolve_effective_github_https_url( + run, '/work', 'ssh://git@github.com/owner/repo.git', push=False, + ) + assert url == 'https://github.com/owner/repo.git' + assert any( + '-c' in cmd and any( + isinstance(part, str) and part.startswith('remote.') and '.url=' in part + for part in cmd + ) + for cmd in seen + ) + + +@pytest.mark.no_pg +def test_pushurl_with_embedded_credentials_is_rejected_without_leaking(): + secret = 'leak-me-not' + remotes = _remote_script( + fetch_url=SAFE_ORIGIN, + push_url=f'https://token:{secret}@github.com/owner/repo.git', + ) + def run(argv): + handled = remotes(argv) + assert handled is not None + return handled + with pytest.raises(GitHubHttpsRemoteError, match='must not contain credentials') as excinfo: + validate_repo_remote(run, '/work', 'origin') + assert secret not in str(excinfo.value) + + +@pytest.mark.no_pg +def test_push_instead_of_rewrite_to_credential_url_is_rejected(): + secret = 'push-rewrite-secret' + remotes = _remote_script( + fetch_url=SAFE_ORIGIN, + push_url=SAFE_ORIGIN, + push_instead_of=[(f'https://bot:{secret}@github.com/', 'https://github.com/')], + ) + def run(argv): + handled = remotes(argv) + assert handled is not None + return handled + with pytest.raises(GitHubHttpsRemoteError, match='must not contain credentials') as excinfo: + resolve_effective_github_https_url(run, '/work', 'origin', push=True) + assert secret not in str(excinfo.value) + + +@pytest.mark.no_pg +def test_account_runner_rejects_unsafe_remote_before_network_git(): + secret = 'before-network' + remotes = _remote_script(fetch_url=f'https://user:{secret}@github.com/owner/repo.git') + network = [] + def runner(argv): + handled = remotes(argv) + if handled is not None: + return handled + if 'gh' in argv: + return Completed(0, 'WorkerOne', '') + git = _git_payload(argv) + if git and git[0] == 'git' and any(v in git for v in ('fetch', 'push')): + network.append(git) + return Completed(0, '', '') + scoped = Account('one', 'WorkerOne', '/accounts/one', IDENTITY).runner(runner, require_git=True) + with pytest.raises(GitHubHttpsRemoteError) as excinfo: + scoped(['git', '-C', '/work', 'fetch', '--', 'origin']) + assert network == [] + assert secret not in str(excinfo.value) + # Local metadata still works without remote validation. + scoped(['git', '-C', '/work', 'rev-parse', 'HEAD']) + scoped(['git', '-C', '/work', 'status', '--porcelain']) + + +@pytest.mark.no_pg +def test_account_runner_allows_safe_fetch_and_blocks_ssh_transport_url(): + remotes = _remote_script() + seen = [] + def runner(argv): + handled = remotes(argv) + if handled is not None: + return handled + if 'gh' in argv: + return Completed(0, 'WorkerOne', '') + seen.append(_git_payload(argv)) + return Completed(0, '', '') + scoped = Account('one', 'WorkerOne', '/accounts/one', IDENTITY).runner(runner, require_git=True) + scoped(['git', '-C', '/work', 'fetch', '--', 'origin']) + assert any('fetch' in cmd for cmd in seen) + with pytest.raises(GitHubHttpsRemoteError, match='HTTPS GitHub'): + scoped(['git', '-C', '/work', 'fetch', '--', 'git@github.com:owner/repo.git']) + scoped(['git', '-C', '/work', 'push', '--', 'origin', 'HEAD:refs/heads/feature']) + scoped(['git', '-C', '/work', 'push', '--set-upstream', 'origin', 'feature']) + + +@pytest.mark.no_pg +@pytest.mark.parametrize('argv', [ + ['git', '-C', '/work', 'fetch'], + ['git', '-C', '/work', 'fetch', '--all'], + ['git', '-C', '/work', 'fetch', '--multiple', 'origin', 'other'], + ['git', '-C', '/work', 'fetch', '--repo=https://github.com/other/repo.git', 'origin'], + ['git', '-C', '/work', 'push'], + ['git', '-C', '/work', 'push', 'HEAD:refs/heads/feature'], + ['git', '-C', '/work', 'pull'], +]) +def test_account_runner_refuses_implicit_or_multi_target_network_forms(argv): + remotes = _remote_script() + network = [] + def runner(cmd): + handled = remotes(cmd) + if handled is not None: + return handled + if 'gh' in cmd: + return Completed(0, 'WorkerOne', '') + git = _git_payload(cmd) + if git and git[0] == 'git' and any(v in git for v in ('fetch', 'push', 'pull')): + network.append(git) + return Completed(0, '', '') + scoped = Account('one', 'WorkerOne', '/accounts/one', IDENTITY).runner(runner, require_git=True) + with pytest.raises(GitHubHttpsRemoteError): + scoped(argv) + assert network == [] + + +@pytest.mark.no_pg +def test_container_mergeable_uses_explicit_fork_target_without_git_identity(tmp_path): + data = { + 'accounts': {'container': { + 'login': 'ContainerWorker', 'gh_config_dir': '/home/worker/.config/gh', + 'command_prefix': ['docker', 'exec', '-i', 'worker-container'], + 'worktree_paths': {'/srv/worker/data': '/data'}, + }}, + 'sessions': {'s1': 'container'}, + } + (tmp_path / 'github-accounts.json').write_text(json.dumps(data)) + gh_calls = [] + def runner(argv): + assert argv[:5] == ['docker', 'exec', '-i', 'worker-container', 'env'] + if 'git' in argv: + raise AssertionError('explicit fork PR must not require git') + idx = argv.index('gh') + command = argv[idx:] + if command[:3] == ['gh', 'api', 'user']: + return Completed(0, 'ContainerWorker', '') + gh_calls.append(command) + if command[:3] == ['gh', 'pr', 'view']: + assert command[:6] == ['gh', 'pr', 'view', '7', '--repo', 'upstream/product'] + return Completed(0, json.dumps({ + 'mergeable': 'MERGEABLE', 'state': 'OPEN', + 'url': 'https://example.invalid/p/7', 'number': 7, + 'headRefOid': 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa', + }), '') + if command[:3] == ['gh', 'pr', 'checks']: + assert command[command.index('--repo') + 1] == 'upstream/product' + return Completed(0, '[]', '') + raise AssertionError(f'unexpected argv: {argv}') + scoped = load_accounts(tmp_path).for_session('s1').runner(runner, require_git=False) + evidence = measure_mergeable( + cwd='/srv/worker/data/repo', + runner=scoped, + repo='upstream/product', + number=7, + ) + assert 'mergeable' in evidence + assert gh_calls + for call in gh_calls: + assert '--repo' in call + assert call[call.index('--repo') + 1] == 'upstream/product' + + +@pytest.mark.no_pg +def test_container_mergeable_derives_branch_with_mapped_git_c(tmp_path): + data = { + 'accounts': {'container': { + 'login': 'ContainerWorker', 'gh_config_dir': '/home/worker/.config/gh', + 'git': {'name': 'Worker', 'email': 'worker@example.com', 'signing_format': 'ssh', 'signing_key': '/home/worker/.ssh/key'}, + 'command_prefix': ['docker', 'exec', '-i', 'worker-container'], + 'worktree_paths': {'/srv/worker/data': '/data'}, + }}, + 'sessions': {'s1': 'container'}, + } + (tmp_path / 'github-accounts.json').write_text(json.dumps(data)) + remotes = _remote_script() + gh_calls = [] + def runner(argv): + assert argv[:5] == ['docker', 'exec', '-i', 'worker-container', 'env'] + handled = remotes(argv) + if handled is not None: + return handled + git = _git_payload(argv) + if git and git[0] == 'git' and 'rev-parse' in git and '--abbrev-ref' in git: + assert git[git.index('-C') + 1] == '/data/repo' + return Completed(0, 'feature\n', '') + idx = argv.index('gh') + command = argv[idx:] + if command[:3] == ['gh', 'api', 'user']: + return Completed(0, 'ContainerWorker', '') + gh_calls.append(command) + if command[:3] == ['gh', 'pr', 'view']: + assert command[:6] == ['gh', 'pr', 'view', 'feature', '--repo', 'owner/repo'] + return Completed(0, json.dumps({ + 'mergeable': 'MERGEABLE', 'state': 'OPEN', + 'url': 'https://example.invalid/p/1', 'number': 1, + 'headRefOid': 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa', + }), '') + if command[:3] == ['gh', 'pr', 'checks']: + assert command[command.index('--repo') + 1] == 'owner/repo' + return Completed(0, '[]', '') + return Completed(0, '', '') + scoped = load_accounts(tmp_path).for_session('s1').runner(runner, require_git=True) + evidence = measure_mergeable(cwd='/srv/worker/data/repo', runner=scoped) + assert 'mergeable' in evidence + assert gh_calls + for call in gh_calls: + assert '--repo' in call + assert call[call.index('--repo') + 1] == 'owner/repo' + + @pytest.mark.no_pg @pytest.mark.parametrize('key,value', [('command_prefix', False), ('worktree_paths', []), ('command_prefix', ['']), ('worktree_paths', {'/a': '/b/../c'})]) def test_invalid_executor_configuration_is_rejected(tmp_path, key, value): diff --git a/tests/test_run.py b/tests/test_run.py index a37b990..543ac5c 100644 --- a/tests/test_run.py +++ b/tests/test_run.py @@ -615,7 +615,7 @@ def fake_push(*, cwd: str, runner): # type: ignore[no-untyped-def] monkeypatch.setattr( "agent_cli.git_act.measure_mergeable", - lambda *, cwd, runner, expected_head=None: "ok", + lambda *, cwd, runner, expected_head=None, repo=None, number=None: "ok", ) run(tmp_path, ["run", "--task", tid]) capsys.readouterr() From c74b72821b31b75fbfadea33175d8dadbfd3dc71 Mon Sep 17 00:00:00 2001 From: Jonny Luca <320529100+JonnyLuca@users.noreply.github.com> Date: Sun, 6 Sep 2026 20:41:43 +0000 Subject: [PATCH 3/8] Correct mergeability test doubles for explicit repository arguments. --- tests/test_git_act.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/test_git_act.py b/tests/test_git_act.py index 393779d..2d1670a 100644 --- a/tests/test_git_act.py +++ b/tests/test_git_act.py @@ -262,7 +262,7 @@ def runner(argv: list[str]) -> Completed: origin = _origin_resolution(argv) if origin is not None: return origin - if argv[:5] == _mergeable_view_argv(): + if argv == [*_mergeable_view_argv(), "--json", "mergeable,state,url,number,headRefOid"]: assert argv[argv.index("--repo") + 1] == REPO return Completed( 0, @@ -293,7 +293,7 @@ def runner(argv: list[str]) -> Completed: origin = _origin_resolution(argv) if origin is not None: return origin - if argv[:5] == _mergeable_view_argv(): + if argv == [*_mergeable_view_argv(), "--json", "mergeable,state,url,number,headRefOid"]: return Completed( 0, json.dumps( @@ -330,7 +330,7 @@ def runner(argv: list[str]) -> Completed: origin = _origin_resolution(argv) if origin is not None: return origin - if argv[:5] == _mergeable_view_argv(): + if argv == [*_mergeable_view_argv(), "--json", "mergeable,state,url,number,headRefOid"]: return Completed( 0, json.dumps( @@ -355,7 +355,7 @@ def runner(argv: list[str]) -> Completed: origin = _origin_resolution(argv) if origin is not None: return origin - if argv[:5] == _mergeable_view_argv(): + if argv == [*_mergeable_view_argv(), "--json", "mergeable,state,url,number,headRefOid"]: return Completed( 0, json.dumps( @@ -386,7 +386,7 @@ def runner(argv: list[str]) -> Completed: origin = _origin_resolution(argv) if origin is not None: return origin - if argv[:5] == _mergeable_view_argv(): + if argv == [*_mergeable_view_argv(), "--json", "mergeable,state,url,number,headRefOid"]: return Completed( 0, json.dumps( @@ -417,7 +417,7 @@ def runner(argv: list[str]) -> Completed: origin = _origin_resolution(argv) if origin is not None: return origin - if argv[:5] == _mergeable_view_argv(): + if argv == [*_mergeable_view_argv(), "--json", "mergeable,state,url,number,headRefOid"]: return Completed( 0, json.dumps( @@ -448,7 +448,7 @@ def runner(argv: list[str]) -> Completed: origin = _origin_resolution(argv) if origin is not None: return origin - if argv[:5] == _mergeable_view_argv(): + if argv == [*_mergeable_view_argv(), "--json", "mergeable,state,url,number,headRefOid"]: return Completed( 0, json.dumps( @@ -473,7 +473,7 @@ def runner(argv: list[str]) -> Completed: origin = _origin_resolution(argv) if origin is not None: return origin - if argv[:5] == _mergeable_view_argv(): + if argv == [*_mergeable_view_argv(), "--json", "mergeable,state,url,number,headRefOid"]: return Completed( 0, json.dumps( From 7025489c5a9b66752b56742f14a80705f99d8406 Mon Sep 17 00:00:00 2001 From: Jonny Luca <320529100+JonnyLuca@users.noreply.github.com> Date: Sun, 6 Sep 2026 21:00:20 +0000 Subject: [PATCH 4/8] Reject ambiguous Git transfer options and redact malformed remote errors. --- docs/github-accounts.md | 9 ++ src/agent_cli/github_accounts.py | 164 +++++++++++++------------------ tests/test_github_accounts.py | 75 ++++++++++++++ 3 files changed, 153 insertions(+), 95 deletions(-) diff --git a/docs/github-accounts.md b/docs/github-accounts.md index 3a06c5e..1adff0a 100644 --- a/docs/github-accounts.md +++ b/docs/github-accounts.md @@ -98,3 +98,12 @@ never load it. One reachable example is `agent a38` visibility lookup login. Configurable AI accounts and roles are part of the [empty-default requirement](../DESIGN.md#198-configuration-starts-empty) and are not implemented by this manifest. + +Transfer options are deliberately limited to the explicit allowlists in +`github_accounts.py`. Unknown options (including custom receive/upload programs), +implicit or multiple repositories, and per-command global configuration/context +overrides are rejected rather than guessed. A transfer may use one mapped `-C` +working directory; validation and execution use that same directory. Automatic +submodule transfers are disabled so a validated parent remote does not authorize +another remote. Other Git commands are not a sandboxed command interface; only +trusted static scripts may supply executor argv. diff --git a/src/agent_cli/github_accounts.py b/src/agent_cli/github_accounts.py index a52e12b..dcc13b9 100644 --- a/src/agent_cli/github_accounts.py +++ b/src/agent_cli/github_accounts.py @@ -34,11 +34,21 @@ "--git-dir", "--work-tree", "--namespace", "--super-prefix", "--config-env", "--buffered-output-size", }) -_FETCH_PUSH_OPTS_WITH_ARG = frozenset({ - "-o", "--upload-pack", "--exec", "--depth", "--shallow-since", "--shallow-exclude", - "--deepen", "--negotiation-tip", "--jobs", "--server-option", "--recv-pack", - "--push-option", "--repo", -}) +_TRANSFER_FLAGS = { + "fetch": frozenset({"--prune", "-p", "--tags", "-t", "--no-tags", "-n", + "--quiet", "-q", "--verbose", "-v", "--dry-run", "--no-recurse-submodules"}), + "push": frozenset({"--set-upstream", "-u", "--dry-run", "-n", "--porcelain", + "--quiet", "-q", "--verbose", "-v", "--atomic"}), + "pull": frozenset({"--ff-only", "--no-rebase", "--quiet", "-q", "--verbose", "-v"}), + "clone": frozenset({"--no-checkout", "-n", "--bare", "--single-branch", + "--no-single-branch", "--quiet", "-q", "--verbose", "-v"}), +} +_TRANSFER_VALUE_FLAGS = { + "fetch": frozenset({"--depth", "--deepen", "--shallow-since", "--shallow-exclude", "--filter"}), + "push": frozenset(), + "pull": frozenset({"--depth"}), + "clone": frozenset({"--depth", "--branch", "-b", "--filter"}), +} class AccountError(StoreError): @@ -67,7 +77,10 @@ def ensure_github_https_remote(url: str) -> str: if not isinstance(url, str) or not url.strip() or "\x00" in url or "\n" in url or "\r" in url: raise GitHubHttpsRemoteError("remote URL is unsafe") text = url.strip() - parsed = urlparse(text) + try: + parsed = urlparse(text) + except ValueError: + raise GitHubHttpsRemoteError("remote URL is unsafe") from None if parsed.scheme != "https": raise GitHubHttpsRemoteError("remote must be HTTPS GitHub") if parsed.username is not None or parsed.password is not None: @@ -237,28 +250,6 @@ def _split_git_command(git_args: list[str]) -> tuple[str | None, list[str]]: return None, [] -def _positionals(rest: list[str], *, opts_with_arg: frozenset[str]) -> list[str]: - if "--" in rest: - return [item for item in rest[rest.index("--") + 1 :] if item] - index = 0 - out: list[str] = [] - while index < len(rest): - arg = rest[index] - if arg.startswith("-"): - name = arg.split("=", 1)[0] - if arg.startswith("--") and "=" in arg: - index += 1 - continue - if arg in opts_with_arg or name in opts_with_arg: - index += 2 - continue - index += 1 - continue - out.append(arg) - index += 1 - return out - - def _git_c_path(git_args: list[str]) -> str | None: if "-C" not in git_args: return None @@ -268,72 +259,50 @@ def _git_c_path(git_args: list[str]) -> str | None: return git_args[index] -def _args_before_double_dash(rest: list[str]) -> list[str]: - if "--" in rest: - return rest[: rest.index("--")] - return rest - - -def _has_flag(rest: list[str], name: str) -> bool: - for arg in _args_before_double_dash(rest): - if arg == name or arg.startswith(name + "="): - return True - return False - - -def _option_value(rest: list[str], name: str) -> str | None: - args = _args_before_double_dash(rest) - index = 0 - while index < len(args): - arg = args[index] - if arg.startswith(name + "="): - return arg.split("=", 1)[1] - if arg == name: - if index + 1 >= len(args): - raise GitHubHttpsRemoteError("unsupported git network command form") - return args[index + 1] - index += 1 - return None - - def _network_remote_targets(git_args: list[str]) -> list[str] | None: - """Return the single explicit remote/URL to validate, or None if not network. - - Supported transfer forms (explicit single repository argument): - - ``git fetch -- origin`` / ``git fetch origin [...]`` - - ``git push -- origin HEAD:refs/heads/feature`` - - ``git push --set-upstream origin feature`` / ``git push -u origin feature`` - - ``git pull`` with an explicit remote - - ``git clone `` - - Implicit default-remote forms, ``fetch --all`` / ``--multiple``, and - ``--repo`` combined with a different positional repository are rejected. - Local metadata commands are not treated as network transfers. + """Accept only explicitly supported single-remote transfers. + + Unknown options fail closed rather than guessing whether the next token is + their value or a repository. Network calls permit only an optional single + global -C; configuration/context overrides cannot differ between validation + and execution. Other local Git commands remain available. """ verb, rest = _split_git_command(git_args) - if verb is None or verb not in _NETWORK_GIT: + if verb not in _NETWORK_GIT: return None - if verb == "clone": - positionals = _positionals(rest, opts_with_arg=_FETCH_PUSH_OPTS_WITH_ARG | _GIT_OPTS_WITH_ARG) - if not positionals: - raise GitHubHttpsRemoteError("git clone requires a repository URL") - return [positionals[0]] - if verb == "fetch" and (_has_flag(rest, "--all") or _has_flag(rest, "--multiple")): - raise GitHubHttpsRemoteError("unsupported git fetch form") - if verb == "pull" and (_has_flag(rest, "--all") or _has_flag(rest, "--multiple")): - raise GitHubHttpsRemoteError("unsupported git pull form") - repo_opt = _option_value(rest, "--repo") - positionals = _positionals(rest, opts_with_arg=_FETCH_PUSH_OPTS_WITH_ARG) - if repo_opt is not None: - if positionals: - raise GitHubHttpsRemoteError("unsupported git network command form") - if not repo_opt.strip(): - raise GitHubHttpsRemoteError("unsupported git network command form") - return [repo_opt] - if not positionals: + leading = git_args[:len(git_args) - len(rest) - 1] + if leading and (len(leading) != 2 or leading[0] != "-C"): + raise GitHubHttpsRemoteError("unsupported git network configuration") + operands: list[str] = [] + index = 0 + while index < len(rest): + arg = rest[index] + if arg == "--": + operands.extend(rest[index + 1:]) + break + if not arg.startswith("-"): + operands.append(arg) + index += 1 + continue + if arg in _TRANSFER_FLAGS[verb]: + index += 1 + continue + name, equal, value = arg.partition("=") + if name not in _TRANSFER_VALUE_FLAGS[verb]: + raise GitHubHttpsRemoteError("unsupported git network option") + if not equal: + index += 1 + if index >= len(rest): + raise GitHubHttpsRemoteError("missing git network option value") + value = rest[index] + if not value or value.startswith("-"): + raise GitHubHttpsRemoteError("invalid git network option value") + index += 1 + if not operands or not operands[0] or operands[0].startswith("-"): raise GitHubHttpsRemoteError(f"git {verb} requires an explicit remote") - first = positionals[0] - # Refspec without a repository uses branch.remote / remote.pushDefault — unsupported. + if verb == "clone" and len(operands) > 2: + raise GitHubHttpsRemoteError("unsupported git clone form") + first = operands[0] if _looks_like_refspec(first) and not _looks_like_url(first): raise GitHubHttpsRemoteError(f"git {verb} requires an explicit remote") return [first] @@ -386,23 +355,28 @@ def scoped(argv: list[str]) -> Completed: f"GIT_COMMITTER_EMAIL={identity['email']}", ]) git_args = list(argv[1:]) - if "-C" in git_args: - index = git_args.index("-C") + 1 - if index >= len(git_args): - raise AccountError("git -C requires a worktree path") - git_args[index] = _map_worktree_path(Path(git_args[index]), self.worktree_paths) targets = _network_remote_targets(git_args) if targets is not None: + # Keep host cwd until each nested call maps it exactly once. cwd = _git_c_path(git_args) for target in targets: validate_repo_remote(scoped, cwd, target) + if "-C" in git_args: + index = git_args.index("-C") + 1 + if index >= len(git_args): + raise AccountError("git -C requires a worktree path") + git_args[index] = _map_worktree_path(Path(git_args[index]), self.worktree_paths) + network_config = ( + ["-c", "fetch.recurseSubmodules=false", "-c", "push.recurseSubmodules=no", + "-c", "submodule.recurse=false"] if targets is not None else [] + ) command = [ "git", "-c", "core.askPass=", "-c", "http.extraHeader=", "-c", "http.https://github.com/.extraHeader=", "-c", "credential.helper=", "-c", "credential.helper=!gh auth git-credential", "-c", f"user.name={identity['name']}", "-c", f"user.email={identity['email']}", "-c", "commit.gpgsign=true", "-c", f"gpg.format={identity['signing_format']}", - "-c", f"user.signingkey={identity['signing_key']}", *git_args, + "-c", f"user.signingkey={identity['signing_key']}", *network_config, *git_args, ] return base([*self.command_prefix, *prefix, *command]) diff --git a/tests/test_github_accounts.py b/tests/test_github_accounts.py index fcf972f..d12d7f2 100644 --- a/tests/test_github_accounts.py +++ b/tests/test_github_accounts.py @@ -651,3 +651,78 @@ def test_merge_watch_refuses_an_account_change(tmp_path): store.write('activity', 'update', 'a1', row) def forbidden(argv): pytest.fail('Changed binding must not reach GitHub') assert scan_merged(store, forbidden) == ([], 1) + + +@pytest.mark.no_pg +@pytest.mark.parametrize('verb,option', [ + ('push', '--receive-pack'), ('push', '--receive-pack=decoy'), + ('push', '--repo'), ('fetch', '--refmap'), ('pull', '--strategy'), + ('pull', '--onto'), ('clone', '--upload-pack'), ('fetch', '--recurse-submodules'), +]) +def test_unknown_transfer_option_never_reaches_git(verb, option): + calls = [] + def runner(argv): + if 'gh' in argv: + return Completed(0, 'WorkerOne', '') + calls.append(argv) + return Completed(0, '', '') + scoped = Account('one', 'WorkerOne', '/accounts/one', IDENTITY).runner(runner) + with pytest.raises(GitHubHttpsRemoteError, match='unsupported'): + scoped(['git', '-C', '/work', verb, option, 'decoy', 'origin']) + assert calls == [] + + +@pytest.mark.no_pg +@pytest.mark.parametrize('prefix', [ + ['-C', '/work', '-c', 'remote.origin.url=https://probe:synthetic@github.com/owner/repo'], + ['-C', '/work', '-C', '/different'], + ['--git-dir', '/different/.git'], +]) +def test_transfer_context_overrides_are_refused_before_validation(prefix): + calls = [] + def runner(argv): + if 'gh' in argv: + return Completed(0, 'WorkerOne', '') + calls.append(argv) + return Completed(0, '', '') + scoped = Account('one', 'WorkerOne', '/accounts/one', IDENTITY).runner(runner) + with pytest.raises(GitHubHttpsRemoteError, match='unsupported') as exc: + scoped(['git', *prefix, 'push', 'origin', 'feature']) + assert 'synthetic' not in str(exc.value) + assert calls == [] + + +@pytest.mark.no_pg +def test_malformed_unicode_netloc_does_not_disclose_credentials(): + secret = 'synthetic-redaction-probe' + with pytest.raises(GitHubHttpsRemoteError, match='unsafe') as exc: + ensure_github_https_remote(f'https://user:{secret}@github.com\uff1a443/owner/repo') + assert secret not in str(exc.value) + + +@pytest.mark.no_pg +def test_transfer_cwd_is_mapped_once_and_submodule_transfers_are_disabled(): + remotes = _remote_script() + calls = [] + raw_calls = [] + def runner(argv): + raw_calls.append(argv) + if 'gh' in argv: + return Completed(0, 'WorkerOne', '') + command = _git_payload(argv) + calls.append(command) + handled = remotes(argv) + if handled is not None: + return handled + return Completed(0, '', '') + account = Account('one', 'WorkerOne', '/accounts/one', IDENTITY, + worktree_paths=(('/srv', '/data'), ('/data', '/wrong'))) + scoped = account.runner(runner) + scoped(['git', '-C', '/srv/repo', 'fetch', '--depth', '1', 'origin']) + scoped(['git', '-C', '/srv/repo', 'push', '--', 'origin', 'HEAD:refs/heads/feature']) + assert calls + assert all(c[c.index('-C') + 1] == '/data/repo' for c in calls) + pushed = raw_calls[-1] + assert 'fetch.recurseSubmodules=false' in pushed + assert 'push.recurseSubmodules=no' in pushed + assert 'submodule.recurse=false' in pushed From 821fda40db57bfa9ff14ebb0b81c42afde76f049 Mon Sep 17 00:00:00 2001 From: Jonny Luca <320529100+JonnyLuca@users.noreply.github.com> Date: Sun, 6 Sep 2026 21:32:29 +0000 Subject: [PATCH 5/8] Require every effective remote URL to identify the same repository. --- docs/github-accounts.md | 4 ++++ src/agent_cli/github_accounts.py | 5 ++-- tests/test_github_accounts.py | 39 ++++++++++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 2 deletions(-) diff --git a/docs/github-accounts.md b/docs/github-accounts.md index 1adff0a..38fbdd5 100644 --- a/docs/github-accounts.md +++ b/docs/github-accounts.md @@ -107,3 +107,7 @@ working directory; validation and execution use that same directory. Automatic submodule transfers are disabled so a validated parent remote does not authorize another remote. Other Git commands are not a sandboxed command interface; only trusted static scripts may supply executor argv. + +Every effective URL returned for a named remote must identify the same GitHub +owner/repository, case-insensitively, including every additional push URL. +Fetch and push URL lists must also agree before a transfer is allowed. diff --git a/src/agent_cli/github_accounts.py b/src/agent_cli/github_accounts.py index dcc13b9..2827e84 100644 --- a/src/agent_cli/github_accounts.py +++ b/src/agent_cli/github_accounts.py @@ -208,8 +208,9 @@ def resolve_effective_github_https_url( urls = _explicit_url_get_urls(run, cwd, remote, push=push) else: urls = _remote_get_urls(run, cwd, remote, push=push) - for raw in urls: - ensure_github_https_remote(raw) + repositories = {ensure_github_https_remote(raw).casefold() for raw in urls} + if len(repositories) != 1: + raise GitHubHttpsRemoteError("remote URLs resolve to different GitHub repositories") return urls[0] diff --git a/tests/test_github_accounts.py b/tests/test_github_accounts.py index d12d7f2..b1facfc 100644 --- a/tests/test_github_accounts.py +++ b/tests/test_github_accounts.py @@ -2,6 +2,7 @@ import json import os +import subprocess import sys from pathlib import Path @@ -726,3 +727,41 @@ def runner(argv): assert 'fetch.recurseSubmodules=false' in pushed assert 'push.recurseSubmodules=no' in pushed assert 'submodule.recurse=false' in pushed + + +@pytest.mark.no_pg +@pytest.mark.parametrize('remote_field', ['url', 'pushurl']) +@pytest.mark.parametrize('same_repository', [False, True]) +def test_all_real_git_remote_urls_must_identify_one_repository(tmp_path, monkeypatch, remote_field, same_repository): + monkeypatch.setenv('GIT_CONFIG_GLOBAL', os.devnull) + monkeypatch.setenv('GIT_CONFIG_SYSTEM', os.devnull) + monkeypatch.setenv('GIT_CONFIG_NOSYSTEM', '1') + repo = tmp_path / 'repo' + subprocess.run(['git', 'init', str(repo)], check=True, capture_output=True) + def git(*args): + subprocess.run(['git', '-C', str(repo), *args], check=True, capture_output=True) + first = 'https://github.com/Owner/Repo.git' + git('remote', 'add', 'origin', first) + if remote_field == 'pushurl': + git('config', '--add', 'remote.origin.pushurl', first) + second = 'https://github.com/owner/repo' if same_repository else 'https://github.com/other/unrequested.git' + git('config', '--add', f'remote.origin.{remote_field}', second) + transfers = [] + def runner(argv): + if 'gh' in argv: + return Completed(0, 'WorkerOne', '') + command = _git_payload(argv) + if command[3] in {'fetch', 'push'}: + transfers.append(command) + return Completed(0, '', '') + result = subprocess.run(argv, text=True, capture_output=True) + return Completed(result.returncode, result.stdout, result.stderr) + scoped = Account('one', 'WorkerOne', '/accounts/one', IDENTITY).runner(runner) + command = ['git', '-C', str(repo), 'push', '--', 'origin', 'HEAD:refs/heads/feature'] + if same_repository: + scoped(command) + assert len(transfers) == 1 + else: + with pytest.raises(GitHubHttpsRemoteError, match='different GitHub repositories'): + scoped(command) + assert transfers == [] From cfb2e5fa9f0a7c1bbf328464c4844418c2fa0ea7 Mon Sep 17 00:00:00 2001 From: Jonny Luca <320529100+JonnyLuca@users.noreply.github.com> Date: Sun, 6 Sep 2026 21:39:12 +0000 Subject: [PATCH 6/8] Isolate Git transport in multi-URL regression tests. --- tests/test_github_accounts.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/test_github_accounts.py b/tests/test_github_accounts.py index b1facfc..e331649 100644 --- a/tests/test_github_accounts.py +++ b/tests/test_github_accounts.py @@ -751,9 +751,11 @@ def runner(argv): if 'gh' in argv: return Completed(0, 'WorkerOne', '') command = _git_payload(argv) - if command[3] in {'fetch', 'push'}: + operation = command[command.index('-C') + 2:] + if operation == ['push', '--', 'origin', 'HEAD:refs/heads/feature']: transfers.append(command) return Completed(0, '', '') + assert operation[:2] == ['remote', 'get-url'], 'Only Git metadata may execute in this test' result = subprocess.run(argv, text=True, capture_output=True) return Completed(result.returncode, result.stdout, result.stderr) scoped = Account('one', 'WorkerOne', '/accounts/one', IDENTITY).runner(runner) From cb672c7abbb528c8a364d669cb5cfe135f660c23 Mon Sep 17 00:00:00 2001 From: Jonny Luca <320529100+JonnyLuca@users.noreply.github.com> Date: Sun, 6 Sep 2026 22:04:31 +0000 Subject: [PATCH 7/8] Support non-repository clones and clear URL-specific authentication headers. --- docs/github-accounts.md | 8 ++- src/agent_cli/github_accounts.py | 36 ++++++++++++- tests/test_github_accounts.py | 88 +++++++++++++++++++++++++++++++- 3 files changed, 128 insertions(+), 4 deletions(-) diff --git a/docs/github-accounts.md b/docs/github-accounts.md index 38fbdd5..a1a5a07 100644 --- a/docs/github-accounts.md +++ b/docs/github-accounts.md @@ -63,7 +63,13 @@ forms, the runner asks Git for the effective remote URL via and `insteadOf` / `pushInsteadOf` rewrite effects) and rejects credential-bearing, non-HTTPS, or non-`github.com` network remotes. Explicit URL arguments are resolved the same way through a temporary command-scoped -remote. Only transfer forms with one explicit repository argument are accepted +remote for fetch, push, and pull. Clone uses the metadata-only +`git ls-remote --get-url` resolution, which applies clone URL rewrites without +requiring an existing local repository or contacting the remote. Before each +supported transfer, all configured `http.*extraHeader` keys, including +repository-specific URL matches, are reset for that invocation so ambient +Authorization headers cannot override the selected account. +Only transfer forms with one explicit repository argument are accepted (for example `git fetch -- origin`, `git push -- origin HEAD:refs/heads/feature`, and `git push --set-upstream origin feature`); implicit default-remote forms, `fetch --all` / `--multiple`, and `--repo` combined with a different positional diff --git a/src/agent_cli/github_accounts.py b/src/agent_cli/github_accounts.py index 2827e84..cff2cbc 100644 --- a/src/agent_cli/github_accounts.py +++ b/src/agent_cli/github_accounts.py @@ -225,6 +225,32 @@ def validate_repo_remote(run: Runner, cwd: str | None, remote: str) -> str: return fetch_repo +def _validate_clone_url(run: Runner, cwd: str | None, url: str) -> None: + """Resolve clone rewrites without requiring a repository or contacting it.""" + if not _looks_like_url(url): + raise GitHubHttpsRemoteError("git clone requires an explicit HTTPS GitHub URL") + result = run(_git_cwd_argv(cwd, "ls-remote", "--get-url", "--", url)) + if result.returncode != 0: + raise GitHubHttpsRemoteError("cannot resolve clone URL") + urls = _parse_get_url_stdout(result.stdout) + if len(urls) != 1: + raise GitHubHttpsRemoteError("clone URL is unsafe") + ensure_github_https_remote(urls[0]) + + +def _clear_http_headers(run: Runner, cwd: str | None) -> list[str]: + """Reset every configured header key, including more-specific URL matches.""" + result = run(_git_cwd_argv( + cwd, "config", "--null", "--name-only", "--get-regexp", r"^http\..*extraheader$", + )) + if result.returncode not in {0, 1}: + raise AccountError("Cannot isolate Git HTTP headers") + args: list[str] = [] + for key in sorted(set(result.stdout.split("\x00")) - {""}): + args.extend(["-c", f"{key}="]) + return args + + def _skip_git_option(args: list[str], index: int) -> int: arg = args[index] name = arg.split("=", 1)[0] @@ -357,11 +383,16 @@ def scoped(argv: list[str]) -> Completed: ]) git_args = list(argv[1:]) targets = _network_remote_targets(git_args) + header_config: list[str] = [] if targets is not None: # Keep host cwd until each nested call maps it exactly once. cwd = _git_c_path(git_args) for target in targets: - validate_repo_remote(scoped, cwd, target) + if _split_git_command(git_args)[0] == "clone": + _validate_clone_url(scoped, cwd, target) + else: + validate_repo_remote(scoped, cwd, target) + header_config = _clear_http_headers(scoped, cwd) if "-C" in git_args: index = git_args.index("-C") + 1 if index >= len(git_args): @@ -377,7 +408,8 @@ def scoped(argv: list[str]) -> Completed: "-c", "credential.helper=!gh auth git-credential", "-c", f"user.name={identity['name']}", "-c", f"user.email={identity['email']}", "-c", "commit.gpgsign=true", "-c", f"gpg.format={identity['signing_format']}", - "-c", f"user.signingkey={identity['signing_key']}", *network_config, *git_args, + "-c", f"user.signingkey={identity['signing_key']}", + *network_config, *header_config, *git_args, ] return base([*self.command_prefix, *prefix, *command]) diff --git a/tests/test_github_accounts.py b/tests/test_github_accounts.py index e331649..1843d9a 100644 --- a/tests/test_github_accounts.py +++ b/tests/test_github_accounts.py @@ -65,6 +65,8 @@ def handle(argv: list[str]) -> Completed | None: git = _git_payload(argv) if not git or git[0] != 'git': return None + if 'config' in git and '--name-only' in git: + return Completed(1, '', '') if 'remote' not in git or 'get-url' not in git: return None explicit_url = None @@ -755,7 +757,7 @@ def runner(argv): if operation == ['push', '--', 'origin', 'HEAD:refs/heads/feature']: transfers.append(command) return Completed(0, '', '') - assert operation[:2] == ['remote', 'get-url'], 'Only Git metadata may execute in this test' + assert operation[:2] in (['remote', 'get-url'], ['config', '--null']), 'Only Git metadata may execute in this test' result = subprocess.run(argv, text=True, capture_output=True) return Completed(result.returncode, result.stdout, result.stderr) scoped = Account('one', 'WorkerOne', '/accounts/one', IDENTITY).runner(runner) @@ -767,3 +769,87 @@ def runner(argv): with pytest.raises(GitHubHttpsRemoteError, match='different GitHub repositories'): scoped(command) assert transfers == [] + + +@pytest.mark.no_pg +@pytest.mark.parametrize('effective_url', [SAFE_ORIGIN, 'https://other.example/owner/repo']) +def test_clone_from_non_repository_resolves_rewrites_without_network(tmp_path, monkeypatch, effective_url): + config = tmp_path / 'global-config' + monkeypatch.setenv('GIT_CONFIG_GLOBAL', str(config)) + monkeypatch.setenv('GIT_CONFIG_SYSTEM', os.devnull) + monkeypatch.setenv('GIT_CONFIG_NOSYSTEM', '1') + source = 'https://clone.example/owner/repo' + subprocess.run(['git', 'config', '--file', str(config), f'url.{effective_url}.insteadOf', source], + check=True, capture_output=True) + assert not (tmp_path / '.git').exists() + transfers = [] + def runner(argv): + if 'gh' in argv: + return Completed(0, 'WorkerOne', '') + command = _git_payload(argv) + operation = command[command.index('-C') + 2:] + if operation == ['clone', '--', source, 'checkout']: + transfers.append(command) + return Completed(0, '', '') + assert operation[:2] in (['ls-remote', '--get-url'], ['config', '--null']) + result = subprocess.run(argv, text=True, capture_output=True) + return Completed(result.returncode, result.stdout, result.stderr) + scoped = Account('one', 'WorkerOne', '/accounts/one', IDENTITY).runner(runner) + command = ['git', '-C', str(tmp_path), 'clone', '--', source, 'checkout'] + if effective_url == SAFE_ORIGIN: + assert scoped(command).returncode == 0 + assert len(transfers) == 1 + else: + with pytest.raises(GitHubHttpsRemoteError, match='HTTPS GitHub'): + scoped(command) + assert transfers == [] + + +@pytest.mark.no_pg +@pytest.mark.parametrize('scope', ['https://github.com/owner/', SAFE_ORIGIN, 'https://github.com:443/owner/']) +def test_repository_specific_http_headers_are_cleared_before_transfer(tmp_path, monkeypatch, scope): + monkeypatch.setenv('GIT_CONFIG_GLOBAL', os.devnull) + monkeypatch.setenv('GIT_CONFIG_SYSTEM', os.devnull) + monkeypatch.setenv('GIT_CONFIG_NOSYSTEM', '1') + subprocess.run(['git', 'init', str(tmp_path)], check=True, capture_output=True) + subprocess.run(['git', '-C', str(tmp_path), 'remote', 'add', 'origin', SAFE_ORIGIN], check=True, capture_output=True) + subprocess.run(['git', '-C', str(tmp_path), 'config', f'http.{scope}.extraHeader', + 'Authorization: synthetic-other-account'], check=True, capture_output=True) + observed = [] + def runner(argv): + if 'gh' in argv: + return Completed(0, 'WorkerOne', '') + start = argv.index('-C') + 2 + operation = argv[start:] + if operation == ['push', '--', 'origin', 'HEAD:refs/heads/feature']: + # Measure Git's actual URL-match result using the transfer's exact + # configuration, while never executing its network operation. + result = subprocess.run([*argv[:start], 'config', '--get-urlmatch', 'http.extraheader', SAFE_ORIGIN], + text=True, capture_output=True) + assert result.returncode == 0 + observed.append(result.stdout) + return Completed(0, '', '') + assert operation[:2] in (['remote', 'get-url'], ['config', '--null']) + result = subprocess.run(argv, text=True, capture_output=True) + return Completed(result.returncode, result.stdout, result.stderr) + scoped = Account('one', 'WorkerOne', '/accounts/one', IDENTITY).runner(runner) + scoped(['git', '-C', str(tmp_path), 'push', '--', 'origin', 'HEAD:refs/heads/feature']) + assert len(observed) == 1 + assert observed[0].strip() == '' + + +@pytest.mark.no_pg +def test_failed_header_inventory_blocks_transfer_without_exposing_error(): + remotes = _remote_script() + def runner(argv): + if 'gh' in argv: + return Completed(0, 'WorkerOne', '') + if '--name-only' in argv: + return Completed(128, '', 'synthetic-private-config-error') + result = remotes(argv) + assert result is not None, 'No transfer may execute after inventory failure' + return result + scoped = Account('one', 'WorkerOne', '/accounts/one', IDENTITY).runner(runner) + with pytest.raises(AccountError, match='Cannot isolate Git HTTP headers') as exc: + scoped(['git', '-C', '/work', 'push', '--', 'origin', 'HEAD:refs/heads/feature']) + assert 'synthetic-private-config-error' not in str(exc.value) From 534d71903fe0ee7e59effb81f148f68e94a02528 Mon Sep 17 00:00:00 2001 From: Jonny Luca <320529100+JonnyLuca@users.noreply.github.com> Date: Mon, 7 Sep 2026 05:53:11 +0000 Subject: [PATCH 8/8] Preserve the execution account when retrying rejected gate reviews. --- src/agent_cli/main.py | 5 +++++ tests/test_cli.py | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+) diff --git a/src/agent_cli/main.py b/src/agent_cli/main.py index d940bff..e2d7652 100644 --- a/src/agent_cli/main.py +++ b/src/agent_cli/main.py @@ -1180,6 +1180,11 @@ def _settled() -> bool: # An errored row is not settled: the executor gave up on it, so a later # `gate record` has to hand it back rather than treat it as delivered. row = store.row("activity", activity_id) + # Preserve the executor's pinned identity from the authoritative read + # under the lock, including recovery from a stale initial insert choice. + payload.pop("execution_account", None) + if row is not None and "execution_account" in row: + payload["execution_account"] = row["execution_account"] return row is not None and row.get("execution_status") != "error" payload = { diff --git a/tests/test_cli.py b/tests/test_cli.py index 2614c6a..94dc525 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -2303,6 +2303,48 @@ def test_rejected_gate_requeues_a_comment_the_executor_gave_up_on(tmp_path: Path assert rows[0]["execution_status"] == "pending" +@pytest.mark.parametrize('stale_read', [False, True]) +def test_gate_retry_preserves_account_and_refuses_switch(tmp_path, capsys, monkeypatch, stale_read): + from agent_cli.github_act import scan_github + + tid, aid = _seed_pr_review_gate(tmp_path, capsys) + argv = _gate_argv(tid, aid, 'rejected', '--evidence', 'file.py:1 retry finding') + run(tmp_path, argv) + activity_id = _review_activities(tmp_path)[0]['id'] + binding = {'account': 'original', 'login': 'firstworker'} + store = Store(tmp_path) + try: + row = {k: v for k, v in store.row('activity', activity_id).items() if not k.startswith('_')} + row.update(execution_status='error', execution_account=binding) + store.write('activity', 'update', activity_id, row) + finally: + store.close() + (tmp_path / 'github-accounts.json').write_text(json.dumps({ + 'accounts': {'replacement': {'login': 'SecondWorker', 'gh_config_dir': '/test/replacement'}}, + 'sessions': {'s': 'replacement'}, + })) + real_row = Store.row + reads = [] + def read(self, table, rid): + if table == 'activity' and rid == activity_id: + reads.append(rid) + if stale_read and len(reads) == 1: + return None + return real_row(self, table, rid) + monkeypatch.setattr(Store, 'row', read) + run(tmp_path, argv) + store = Store(tmp_path) + try: + assert store.row('activity', activity_id)['execution_account'] == binding + assert store.row('activity', activity_id)['execution_status'] == 'pending' + def forbidden(argv): + pytest.fail('Changed account must be refused before any GitHub access') + assert scan_github(store, forbidden) == [f'review.post {activity_id} error'] + assert 'refusing account switch' in store.row('activity', activity_id)['execution_error'] + finally: + store.close() + + def test_rejected_gate_survives_a_stale_existence_read( tmp_path: Path, capsys: pytest.CaptureFixture[str], monkeypatch: pytest.MonkeyPatch ) -> None: