diff --git a/docs/spec/runtime/repos.md b/docs/spec/runtime/repos.md index 83510f18c..7f0264bcc 100644 --- a/docs/spec/runtime/repos.md +++ b/docs/spec/runtime/repos.md @@ -452,22 +452,36 @@ missing: Both are otherwise silent: the tools simply are not wired, and the only symptom is an agent that says it cannot see the code. -## Not in this tier - -**No push path exists anywhere.** The write tier — PR creation, agent-attributed -commits, namespaced branches, operator approval — is a separate follow-up, and -the confinement above is arranged so that adding it is an explicit new capability -rather than a hole that was already open. - -Its groundwork (issue #734) has landed without opening that path: a `repo.write` -grant now exists — distinct from and tighter than `repo`, so a bare `repo` (the -read tier every company sets) never confers it and the catch-all `*` never -confers it — and each binding records whether its bound credential can push, read -from the forge's `permissions.push` at bind time and healed on the next fetch -when it is still unknown. Granting `repo.write` over a read-only credential is -fail-closed: it warns and wires nothing. No tool consumes either yet; they are -what the push follow-up will gate on. - -Also absent: a distinct uid or read-only bind mount for the agent shell (the real -filesystem boundary, named under ["The honest limit"](#the-honest-limit)), and -forges other than GitHub. +## Beyond this tier — the write tier + +The read tier above ships no push path of its own. The **write tier** is a +separate, explicitly-added capability layered on top, arranged so that adding it +is a new opt-in rather than a hole that was already open here. It is built in +three parts: + +- **A `repo.write` grant + credential push-capability** (issue #734). The grant + is distinct from and tighter than `repo`: a bare `repo` (the read tier every + company sets) never confers it, and the catch-all `*` never confers it. Each + binding also records whether its bound credential can push, read from the + forge's `permissions.push` at bind time and healed on the next fetch while it + is still unknown. Granting `repo.write` over a read-only credential is + fail-closed — it warns and wires nothing. +- **`repo_publish`** (issue #735). The agent commits locally in its checkout, + then publishes host-side. The host *fetches* the checkout's committed HEAD into + the mirror on a host-owned `oc//` branch — a fetch never invokes + `receive-pack`, so the read tier's `pre-receive` refusal and its no-push + contract test stay untouched — and, only after the operator approves, pushes + that exact commit to the remote. The agent still holds no credentialed remote + and never pushes; every structural refusal (host-generated namespaced branch, + never a force push, never the default branch, never a ref outside `oc/`) lives + in `RepoManager` where no prompt can reach it. +- **Pull-request creation** (issue #736). After the push, the host opens a pull + request into the repository's default branch, best-effort: a PR that fails to + open leaves the branch on the remote and reports that honestly on the task, + rather than failing the publish. + +Still absent: **signed** commits carrying a per-agent identity key (issue #738, +deferred — plain author/committer attribution ships with the write tier, but +signing waits for a consumer that verifies a signature); a distinct uid or +read-only bind mount for the agent shell (the real filesystem boundary, named +under ["The honest limit"](#the-honest-limit)); and forges other than GitHub. diff --git a/src/harness/brain.rs b/src/harness/brain.rs index 2c9d925aa..1990f177e 100644 --- a/src/harness/brain.rs +++ b/src/harness/brain.rs @@ -392,6 +392,9 @@ impl HarnessBrain { // over an empty ledger is a no-op, which is what every turn that touches // no repository does. let _checkout_janitor = CheckoutJanitor::claim(&self.deps.checkouts); + // Issue #735: a re-dispatched grant is not a task card, so clear any task + // a prior turn stamped — `repo_publish` requires a task and refuses here. + self.deps.checkouts.set_task(None); // Un-streamed, like a dispatched card: this turn is answered by the // bubble returned below, and its transient frames would otherwise // misattribute onto whichever chat thread the console is watching. @@ -633,6 +636,10 @@ impl HarnessBrain { // guard's `Drop` is what deletes the tree on every exit — success, // error, cancel, redirect exhaustion and panic-unwind alike. let _checkout_janitor = CheckoutJanitor::claim(&self.deps.checkouts); + // Issue #735: this is a dispatched card, so `repo_publish` names its + // branch `oc//`. Stamped on the same per-turn cell the + // janitor above claims. + self.deps.checkouts.set_task(Some(card.id.clone())); // Issue #339, same argument for staged workflow references: an operator // chat turn earlier in this cycle may have run a workflow through the // orchestrator's tool, and that run belongs to the conversation, not to @@ -2456,6 +2463,10 @@ impl HarnessBrain { // so it can clone a repository, and the guard's `Drop` // removes it when this turn ends. let _checkout_janitor = CheckoutJanitor::claim(&self.deps.checkouts); + // Issue #735: a conversation is not a task card, so clear any + // task a prior turn stamped — `repo_publish` requires a task + // and refuses on a chat turn (task turns only, this tier). + self.deps.checkouts.set_task(None); // Drive the brain-agnostic delegation seam (issue #176): the // orchestrator turn, its queued delegations, and the CEO-relay // hand-back all run behind the `RunTurn` impl. `HarnessDeps` is diff --git a/src/harness/build.rs b/src/harness/build.rs index 5e9e1e774..da9825c8f 100644 --- a/src/harness/build.rs +++ b/src/harness/build.rs @@ -508,6 +508,8 @@ pub fn build_agent( bindings: deps.repo_bindings.clone().into(), workspace: workspace.clone(), ledger: deps.checkouts.clone(), + agent: manifest_agent.id.clone(), + approvals: deps.approval_requests.clone(), }, )); } @@ -526,33 +528,58 @@ pub fn build_agent( } } - // Repository WRITE tier (issue #734). A distinct, tighter grant than the read - // `repo` above: `grants_repo_write_explicit` matches ONLY the exact + // Repository WRITE tier (issues #734, #735). A distinct, tighter grant than + // the read `repo` above: `grants_repo_write_explicit` matches ONLY the exact // `repo.write`, so a bare `repo` (which every read-tier company writes) and // the catch-all `*` confer nothing here — a company that asked for agents // reading code does not silently get agents pushing it. // - // #734 only *learns and records* whether a bound credential can push (probed - // at bind, re-probed on fetch, stored on `RepoBinding::can_push`); the - // `repo_publish` tool that consumes it lands in #735, so this wires NO tool - // yet. What it does now is the fail-closed half of #247's requirement: when - // `repo.write` is granted but no bound repository has a push-capable - // credential, say so and wire nothing, rather than leaving the operator to - // discover at publish time that the credential they bound was read-only. - // `None` (unknown — unprobed or pre-field) reads as cannot-push here, so only - // a proven `Some(true)` clears the warning. + // FOUR gates, all fail-closed, and the fourth is the one #734 added: an + // explicit `repo.write` grant, a wired manager, at least one binding, AND a + // bound credential that can actually push (`can_push == Some(true)`; `None` — + // unprobed or pre-field — reads as cannot-push). Missing any one wires + // `repo_publish` NOT AT ALL and says which, rather than offering a publish + // that would fail at push time on a read-only credential. + // + // Like the read tier, NOT feature-gated: the mirror and git runner are always + // compiled, and `repo_publish`'s push waits on an operator approval the + // runtime performs, so there is no forge client to gate the tool behind. if crate::company::grants_repo_write_explicit(grants) { - let has_push_capable = deps + let push_capable = deps .repo_bindings .iter() .any(|binding| binding.can_push == Some(true)); - if !has_push_capable { - tracing::warn!( + match (&deps.repos, deps.repo_bindings.is_empty(), push_capable) { + (Some(repos), false, true) => { + tools.push(crate::harness::repo::repo_publish_tool( + crate::harness::repo::RepoToolContext { + repos: repos.clone(), + bindings: deps.repo_bindings.clone().into(), + workspace: workspace.clone(), + ledger: deps.checkouts.clone(), + agent: manifest_agent.id.clone(), + approvals: deps.approval_requests.clone(), + }, + )); + } + (None, _, _) => tracing::warn!( + company = %company, + agent = %manifest_agent.id, + "[build] agent explicitly grants `repo.write` but no repository cache is configured \ + on this host; repo_publish NOT wired (fail-closed)" + ), + (Some(_), true, _) => tracing::warn!( + company = %company, + agent = %manifest_agent.id, + "[build] agent explicitly grants `repo.write` but this company has bound no \ + repositories; repo_publish NOT wired (fail-closed)" + ), + (Some(_), false, false) => tracing::warn!( company = %company, agent = %manifest_agent.id, "[build] agent explicitly grants `repo.write` but no bound repository has a \ - push-capable credential; write tools NOT wired (fail-closed)" - ); + push-capable credential; repo_publish NOT wired (fail-closed)" + ), } } @@ -1643,7 +1670,23 @@ mod tests { /// `deps.repo_bindings` — so the difference between the two is exactly /// "the operator bound something", which is two of the four gate states. fn built_tool_names_with_repos(grants: &[&str], bindings: usize) -> Vec { - built_tool_names_with_repos_on(grants, bindings, crate::store::StorageKind::Mongodb) + built_tool_names_with_repos_cap(grants, bindings, false) + } + + /// [`built_tool_names_with_repos`], with control over whether the bound + /// credentials read as push-capable (issue #735) — the fourth gate the write + /// tier adds. `false` matches every read-tier caller (`can_push: None`). + fn built_tool_names_with_repos_cap( + grants: &[&str], + bindings: usize, + push_capable: bool, + ) -> Vec { + built_tool_names_with_repos_on_cap( + grants, + bindings, + push_capable, + crate::store::StorageKind::Mongodb, + ) } /// [`built_tool_names_with_repos`], with the secret backend spelled out — @@ -1655,6 +1698,20 @@ mod tests { grants: &[&str], bindings: usize, storage_kind: crate::store::StorageKind, + ) -> Vec { + built_tool_names_with_repos_on_cap(grants, bindings, false, storage_kind) + } + + /// Both gates at once. #735 added the push-capability of the bound + /// credential and #752 added the secret backend holding it, independently + /// and to the same helper; a caller that fixes one still has to be able to + /// vary the other, so the two thin wrappers above each pin their own + /// default and this carries the full shape. + fn built_tool_names_with_repos_on_cap( + grants: &[&str], + bindings: usize, + push_capable: bool, + storage_kind: crate::store::StorageKind, ) -> Vec { use crate::runtime::repo_manager::types::RepoBinding; let dir = tempfile::tempdir().expect("tempdir"); @@ -1678,7 +1735,7 @@ mod tests { last_fetched_millis: None, size_bytes: 0, bound_at_millis: 1, - can_push: None, + can_push: if push_capable { Some(true) } else { None }, }) .collect(); let manifest_agent = ManifestAgent { @@ -1833,7 +1890,43 @@ mod tests { let write_granted = built_tool_names_with_repos(&["repo.write"], 1); assert_eq!( write_granted, baseline, - "repo.write must wire only the read pair — no write tool exists until #735" + "repo.write with a non-push-capable credential wires only the read pair" + ); + } + + /// The write tier's fourth gate (issue #735): `repo_publish` is wired only + /// with `repo.write` **and** a push-capable credential, and never by the read + /// `repo` grant. The non-push-capable half is + /// `repo_write_grant_wires_no_tool_beyond_the_read_pair` above. + #[test] + fn repo_write_with_a_push_capable_credential_wires_repo_publish() { + let publish = "repo_publish".to_string(); + + // repo.write + a push-capable credential → repo_publish joins the belt, + // and the read pair is still there (write implies read). + let pushable = built_tool_names_with_repos_cap(&["repo.write"], 1, true); + assert!( + pushable.contains(&publish), + "a push-capable `repo.write` must wire repo_publish: {pushable:?}" + ); + assert!( + pushable.contains(&"repo_checkout".to_string()) + && pushable.contains(&"repo_pr".to_string()), + "the read pair must still be wired: {pushable:?}" + ); + + // repo.write but a read-only credential → fail-closed, no publish. + let read_only = built_tool_names_with_repos_cap(&["repo.write"], 1, false); + assert!( + !read_only.contains(&publish), + "a read-only credential must not wire repo_publish: {read_only:?}" + ); + + // A bare `repo` never confers it, push-capable credential or not. + let bare = built_tool_names_with_repos_cap(&["repo"], 1, true); + assert!( + !bare.contains(&publish), + "bare `repo` (the read tier) must never wire repo_publish: {bare:?}" ); } diff --git a/src/harness/repo.rs b/src/harness/repo.rs index fe5585ae2..8ba8cee33 100644 --- a/src/harness/repo.rs +++ b/src/harness/repo.rs @@ -78,6 +78,8 @@ use serde_json::{Value, json}; use crate::Result; use crate::error::OpenCompanyError; +use crate::harness::policy::ApprovalRequest; +use crate::ports::types::{Effect, EffectGroup}; use crate::runtime::RepoManager; use crate::runtime::repo_manager::types::{RepoBinding, parse_repo_url}; use crate::runtime::repo_manager::{dir_bytes, git, human_bytes, validate_ref}; @@ -88,6 +90,10 @@ pub const REPO_CHECKOUT_TOOL: &str = "repo_checkout"; /// Tool name: read a pull request's metadata and unified diff. pub const REPO_PR_TOOL: &str = "repo_pr"; +/// Tool name: publish the branch the agent committed, host-side, for operator +/// approval (issue #735). +pub const REPO_PUBLISH_TOOL: &str = "repo_publish"; + /// The workspace subdirectory every checkout and diff spill lands in. /// /// One directory, named once, because three separate things key off it: the @@ -132,9 +138,28 @@ const HOST_TRUNCATION_MARKER: &str = "[truncated:"; #[derive(Clone, Debug, Default)] pub struct CheckoutLedger { inner: Arc>>, + /// The task the current turn runs under (issue #735) — what names the + /// `oc//` branch `repo_publish` pushes to. Held on the cell + /// the repository tools already share and the turn's entry point already + /// claims, rather than as a second `HarnessDeps` field: the task and the + /// checkouts have the exact same per-turn lifetime. `None` on a turn with no + /// card, where `repo_publish` refuses (issue #735 ships task turns only). + task: Arc>>, } impl CheckoutLedger { + /// Stamps the task the current turn runs under (issue #735). Called by the + /// turn's entry point alongside the janitor claim; the janitor's path purge + /// does not touch it. + pub fn set_task(&self, task: Option) { + *self.task.lock().expect("checkout ledger task") = task; + } + + /// The task the current turn runs under, if any (issue #735). + pub fn task(&self) -> Option { + self.task.lock().expect("checkout ledger task").clone() + } + /// Records a path this turn created. pub fn record(&self, path: PathBuf) { let mut guard = self.inner.lock().expect("checkout ledger"); @@ -389,6 +414,33 @@ fn file_url(path: &Path) -> String { format!("file://{}", path.display()) } +/// Sets a fresh checkout's commit identity to the agent's seat (issue #735). +/// +/// git makes commits with the repository's own `user.name`/`user.email`, so +/// setting them here — before the agent can commit through `git_operations` — +/// is what attributes the branch it may later publish to the agent rather than +/// to a shared machine identity. The address is synthetic and non-routable; it +/// exists to identify, not to receive mail. Best-effort: a config that will not +/// set is logged, and a checkout that is only ever read is unaffected either way. +async fn attribute_checkout(dest: &Path, agent: &str) { + for (key, value) in [ + ("user.name", agent.to_string()), + ("user.email", format!("{agent}@agents.opencompany.local")), + ] { + match git::run(dest, &["config", key, &value], None, None).await { + Ok(out) if out.ok => {} + Ok(out) => tracing::debug!( + agent, + "[repo] could not set {key} on the checkout: {}", + first_line(&out.stderr) + ), + Err(err) => { + tracing::debug!(agent, "[repo] could not set {key} on the checkout: {err}") + } + } + } +} + /// Removes a directory if it exists. Absent is success. async fn remove_dir(path: &Path) -> Result<()> { match tokio::fs::remove_dir_all(path).await { @@ -428,6 +480,14 @@ pub struct RepoToolContext { pub workspace: PathBuf, /// Where this turn's created paths are recorded for deletion. pub ledger: CheckoutLedger, + /// This agent's seat (issue #735). Attributes the commits `repo_publish` + /// pushes and labels the approval the operator sees. + pub agent: String, + /// Where `repo_publish` records the operator approval its push waits on + /// (issue #735) — the shared queue the policy and the brain already drain, + /// handed to the tool the same way `ledger` is. The per-turn task id that + /// names the publish branch rides on [`CheckoutLedger::task`], not here. + pub approvals: crate::harness::policy::ApprovalRequestQueue, } impl RepoToolContext { @@ -632,6 +692,11 @@ impl Tool for RepoCheckoutTool { Ok(head) => head, Err(err) => return Ok(ToolResult::error(err.to_string())), }; + // Attribute any commits the agent makes here to its seat (issue #735), + // set before it can commit via `git_operations`, so a branch it later + // publishes carries "which agent wrote this" in `git log` on the remote. + // Best-effort: a checkout that is only read is unaffected. + attribute_checkout(&dest, &self.context.agent).await; let relative = format!("{CHECKOUT_SUBDIR}/{}", binding.key); let at = match pull_request { @@ -822,5 +887,195 @@ impl Tool for RepoPullRequestTool { } } +// --------------------------------------------------------------------------- +// repo_publish (issue #735) +// --------------------------------------------------------------------------- + +/// Publish the branch the agent committed in its checkout, host-side and gated +/// by operator approval. +/// +/// The two-step shape is the whole design (see [`RepoManager::stage_publish`]). +/// `execute` runs the **reversible** half immediately — it fetches the agent's +/// committed `HEAD` out of the task-scoped checkout and into the mirror on a +/// host-owned `oc//` ref — so the work is durable the instant the +/// tool returns, before the checkout is cleaned up at turn end. The +/// **irreversible** half — the push to the real remote — is not done here. It is +/// recorded as a native [`Effect`] (`agent: None`) that the runtime performs +/// **only after the operator approves**, exactly as `email.send` does. A denied +/// or expired approval never runs it, so the remote is untouched. +/// +/// The agent never pushes and never holds a credentialed remote: both git write +/// directions are host-side in [`RepoManager`], and the branch name is generated +/// there, never taken from the agent. +struct RepoPublishTool { + context: RepoToolContext, +} + +#[async_trait] +impl Tool for RepoPublishTool { + fn name(&self) -> &str { + REPO_PUBLISH_TOOL + } + + fn description(&self) -> &str { + "Publish the commits you made in a checked-out repository as a branch on the company's \ + remote, for the operator to review. USE FOR handing over a change you have committed in a \ + `repo_checkout` working tree — a fix, a patch, a generated file — once it is ready. The \ + push is host-side and needs the operator's approval: this tool stages your commits and \ + asks; nothing reaches the remote until the operator approves, and you will be told it is \ + pending, not done. NOT a way to push to `main` or any branch you name — the branch is \ + chosen for you (`oc//`) and only that branch is ever written. Commit your \ + work first with `git_operations`; an empty or unchanged checkout has nothing to publish." + } + + fn parameters_schema(&self) -> Value { + json!({ + "type": "object", + "properties": { + "repo": { + "type": "string", + "description": "Which bound repository you checked out and committed to, as \ + `owner/name`, its https URL, or the key from the repositories list." + }, + "message": { + "type": "string", + "description": "A short summary of what this publish contains, for the operator \ + reviewing it before it is pushed." + } + }, + "required": ["repo", "message"], + "additionalProperties": false + }) + } + + fn permission_level(&self) -> PermissionLevel { + // Advisory, like every tool in this crate — the real gate is the native + // approval this records (the push waits for it) plus the + // `crate::policy::consequence` declaration that keeps a `readonly` desk + // from reaching this at all. `Write` is the honest claim: approved, it + // moves the agent's commits onto a real remote. + PermissionLevel::Write + } + + async fn execute(&self, args: Value) -> anyhow::Result { + let raw = args.get("repo").and_then(Value::as_str).unwrap_or_default(); + let binding = match self.context.resolve(raw) { + Ok(binding) => binding.clone(), + Err(message) => return Ok(ToolResult::error(message)), + }; + // The tool is wired when ANY bound credential can push, but the agent may + // name a repository whose OWN credential is read-only. Refuse that here — + // before staging and before an operator is asked to approve — rather than + // letting it surface only when the host tries the push (issue #735). + // `None` (unprobed) reads as cannot-push, like everywhere else. + if binding.can_push != Some(true) { + return Ok(ToolResult::error(format!( + "The credential bound for {}/{} is read-only, so it cannot publish. Ask an \ + operator to bind a push-capable credential for this repository.", + binding.owner, binding.repo + ))); + } + let message = args + .get("message") + .and_then(Value::as_str) + .map(str::trim) + .unwrap_or_default(); + if message.is_empty() { + return Ok(ToolResult::error( + "`message` is required: say what this publish contains so the operator can review \ + it before it is pushed." + .to_string(), + )); + } + + // A publish belongs to a task (this tier ships task turns only). A turn + // with no card leaves the task unstamped, and there is no branch to name. + let Some(task) = self.context.ledger.task() else { + return Ok(ToolResult::error( + "Publishing is only available while you are working a task, not in a plain \ + conversation. There is nothing to do here — say so rather than retrying." + .to_string(), + )); + }; + + // The task-scoped checkout the agent committed in. Resolved host-side + // from the workspace and the binding key — never a path the agent typed. + let checkout = self.context.checkout_root().join(&binding.key); + + // Stage the committed work into the mirror now, while the checkout still + // exists. This is the reversible half; it makes the work durable so the + // approved push below does not depend on a checkout that is deleted at + // turn end. + let (branch, head) = match self + .context + .repos + .stage_publish(&binding.key, &checkout, &task) + .await + { + Ok(staged) => staged, + Err(err) => { + return Ok(ToolResult::error(format!( + "Could not stage your work to publish: {err}" + ))); + } + }; + + // Record the irreversible push as a native effect the runtime performs + // on approval. `agent: None` is load-bearing: it is what makes the + // runtime push it itself on approval rather than re-dispatching this + // turn (which would have lost the checkout). The payload is what the + // operator's approval card shows and what `perform_effect` reads. + let effect = Effect { + kind: crate::runtime::cycle::REPO_PUBLISH_EFFECT.to_string(), + group: EffectGroup::Publish, + amount_usd: None, + established_thread: false, + first_time_counterparty: false, + payload: json!({ + "repo": binding.key, + "owner": binding.owner, + "name": binding.repo, + "branch": branch, + // The exact commit this approval is bound to. `perform_effect` + // pushes THIS commit, so a later re-stage of the same task cannot + // change what an approved publish sends. + "head": head, + // The task/card this publish belongs to (issue #736). Carried so + // the runtime can link the opened PR back to it, and post a note + // on it if the push lands but the PR does not open. + "task": task, + "agent": self.context.agent, + "message": message, + }), + agent: None, + run_id: None, + }; + self.context.approvals.push(ApprovalRequest { + tool: REPO_PUBLISH_TOOL.to_string(), + reason: format!( + "publish {}/{} to {branch} for review", + binding.owner, binding.repo + ), + effect, + }); + + Ok(ToolResult::success(format!( + "Staged your commits as `{branch}` and asked the operator to approve publishing them \ + to {}/{}. Nothing has been pushed yet — the push happens only once the operator \ + approves, so tell them it is pending review, not delivered.", + binding.owner, binding.repo + ))) + } +} + +/// Builds the `repo_publish` tool (issue #735). Kept separate from +/// [`repo_tools`] because it is wired behind a strictly tighter gate — the +/// `repo.write` grant and a push-capable credential — decided in +/// [`build_agent`](crate::harness::build::build_agent), where the read tools are +/// wired on the plain `repo` grant. +pub fn repo_publish_tool(context: RepoToolContext) -> Box { + Box::new(RepoPublishTool { context }) +} + #[cfg(test)] mod test; diff --git a/src/harness/repo/test.rs b/src/harness/repo/test.rs index 22413ac0f..2afbdac23 100644 --- a/src/harness/repo/test.rs +++ b/src/harness/repo/test.rs @@ -22,7 +22,9 @@ use async_trait::async_trait; use super::*; use crate::ports::SecretStore; use crate::ports::types::{CompanyId, SecretValue}; -use crate::runtime::repo_manager::types::{PullRequestView, RepoCoordinates, RepoHost, RepoMeta}; +use crate::runtime::repo_manager::types::{ + PullRequestRef, PullRequestView, RepoCoordinates, RepoHost, RepoMeta, +}; /// A scratch directory removed when the test ends. struct Scratch(PathBuf); @@ -109,6 +111,21 @@ impl RepoHost for ScriptedHost { diff: self.diff.clone(), }) } + + async fn create_pull_request( + &self, + _coords: &RepoCoordinates, + _token: &str, + _head: &str, + _base: &str, + _title: &str, + _body: &str, + ) -> crate::Result { + Ok(PullRequestRef { + number: 7, + html_url: "https://github.com/acme/fixture/pull/7".into(), + }) + } } /// Runs git in `cwd`, panicking with its stderr on failure. @@ -219,6 +236,8 @@ fn context( bindings: bindings.into(), workspace, ledger: CheckoutLedger::default(), + agent: "desk".to_string(), + approvals: crate::harness::policy::ApprovalRequestQueue::default(), } } @@ -922,3 +941,135 @@ async fn the_pair_is_wired_under_the_declared_names() { assert_eq!(REPO_CHECKOUT_TOOL, "repo_checkout"); assert_eq!(REPO_PR_TOOL, "repo_pr"); } + +// --------------------------------------------------------------------------- +// repo_publish (issue #735) +// --------------------------------------------------------------------------- + +/// Materializes a checkout at the path `repo_publish` resolves and commits one +/// file there, standing in for an agent that checked out and committed. +async fn committed_checkout(ctx: &RepoToolContext, mirror: &Path, key: &str) { + let dest = ctx.workspace.join(CHECKOUT_SUBDIR).join(key); + materialize(mirror, &dest, Some("main"), None) + .await + .expect("materialize"); + identify(&dest); + std::fs::write(dest.join("FIX.md"), "the fix\n").unwrap(); + git_at(&dest, &["add", "FIX.md"]); + git_at(&dest, &["commit", "--quiet", "-m", "the fix"]); +} + +/// Publishing outside a task refuses — this tier is task turns only, and there +/// is no card to name the branch. Nothing is staged and nothing is queued. +#[tokio::test] +async fn repo_publish_without_a_task_refuses() { + let scratch = Scratch::new("publish-no-task"); + let (manager, mut binding) = bound(&scratch, &["main"]).await; + binding.can_push = Some(true); // push-capable, so the task check is what refuses + let ctx = context(&scratch, manager, vec![binding.clone()]); + ctx.ledger.set_task(None); // a chat turn: no card + let tool = repo_publish_tool(ctx.clone()); + + let result = tool + .execute(json!({ "repo": binding.key, "message": "the fix" })) + .await + .unwrap(); + assert!(result.is_error, "{result:?}"); + assert!(result.text().contains("task"), "{}", result.text()); + assert_eq!( + ctx.approvals.queued(), + 0, + "nothing may be queued when refused" + ); +} + +/// On a task turn, publishing stages the agent's commit onto the mirror's +/// namespaced branch and queues a native (`agent: None`) `repo.publish` approval +/// for the operator. The push itself is NOT done in the tool. +#[tokio::test] +async fn repo_publish_stages_and_queues_a_native_approval() { + let scratch = Scratch::new("publish-queue"); + let (manager, mut binding) = bound(&scratch, &["main"]).await; + binding.can_push = Some(true); + let mirror = manager.mirror_path(&binding.key); + let ctx = context(&scratch, manager, vec![binding.clone()]); + ctx.ledger.set_task(Some("card-1".to_string())); + committed_checkout(&ctx, &mirror, &binding.key).await; + let tool = repo_publish_tool(ctx.clone()); + + let result = tool + .execute(json!({ "repo": binding.key, "message": "the fix" })) + .await + .unwrap(); + assert!(!result.is_error, "{result:?}"); + assert!( + result.text().contains("approve") || result.text().contains("pending"), + "the agent must be told it is pending, not delivered: {}", + result.text() + ); + + // The commit is staged onto the host-owned branch in the mirror... + let staged = git_at(&mirror, &["rev-parse", "refs/heads/oc/acme/card-1"]); + assert!(!staged.is_empty(), "the mirror carries the staged branch"); + + // ...and a single native approval is queued for the push. + let drained = ctx.approvals.drain(16); + assert_eq!( + drained.requests.len(), + 1, + "one approval queued: {drained:?}" + ); + let req = &drained.requests[0]; + assert_eq!(req.tool, REPO_PUBLISH_TOOL); + assert_eq!(req.effect.kind, "repo.publish"); + assert_eq!( + req.effect.agent, None, + "a native effect: the runtime performs the push on approval, not a re-dispatched agent" + ); + assert_eq!( + req.effect.payload.get("branch").and_then(|v| v.as_str()), + Some("oc/acme/card-1"), + "the approval carries the host-generated branch" + ); + // The approval is bound to the exact staged commit, so the push is not at the + // mercy of a later re-stage of the same task. + assert_eq!( + req.effect.payload.get("head").and_then(|v| v.as_str()), + Some(staged.as_str()), + "the approval carries the staged commit id" + ); +} + +/// The tool is wired when any bound credential can push, but a specific +/// repository whose own credential is read-only must be refused before staging — +/// not left to fail when the host tries the push (issue #735). +#[tokio::test] +async fn repo_publish_refuses_a_read_only_binding() { + let scratch = Scratch::new("publish-readonly"); + let (manager, mut binding) = bound(&scratch, &["main"]).await; + binding.can_push = Some(false); // this repository's credential cannot push + let mirror = manager.mirror_path(&binding.key); + let ctx = context(&scratch, manager, vec![binding.clone()]); + ctx.ledger.set_task(Some("card-1".to_string())); + committed_checkout(&ctx, &mirror, &binding.key).await; + let tool = repo_publish_tool(ctx.clone()); + + let result = tool + .execute(json!({ "repo": binding.key, "message": "the fix" })) + .await + .unwrap(); + assert!(result.is_error, "{result:?}"); + assert!( + result.text().contains("read-only"), + "the refusal must name the read-only credential: {}", + result.text() + ); + assert_eq!( + ctx.approvals.queued(), + 0, + "a read-only binding must stage and queue nothing" + ); + // And nothing was staged into the mirror. + let (ok, _) = git_try(&mirror, &["rev-parse", "refs/heads/oc/acme/card-1"]); + assert!(!ok, "no branch may be staged for a read-only binding"); +} diff --git a/src/policy/consequence.rs b/src/policy/consequence.rs index 791ce7728..28cfe54cc 100644 --- a/src/policy/consequence.rs +++ b/src/policy/consequence.rs @@ -625,6 +625,28 @@ const DECLARED: &[Declared] = &[ // permission are separate answers (issue #444). d("repo_checkout", EffectGroup::Other, Reach::Consequence), d("repo_pr", EffectGroup::Other, Reach::Consequence), + // `repo_publish` (issue #735) is classified by what the CALL does, which is + // deliberately NOT what its approval settles. The call stages the agent's + // committed work onto a host-side `oc//` ref in the mirror and + // records an operator approval. It reaches no counterparty, spends nothing, + // and the stage is reversible and never leaves the host — so `Nothing` at the + // tool layer. The irreversible push to the real remote is a separate native + // effect (`repo.publish`, `EffectGroup::Publish`) that the runtime performs + // ONLY on the operator's approval, so *that* effect is where the consequence + // and its gate live — see the `repo.publish` arm of `perform_effect`. + // + // `Nothing`, not `Consequence`, is load-bearing rather than a downgrade: a + // `Consequence` call PARKS under `supervised`, and a parked call whose + // `execute` never ran would have nothing to stage — the checkout it stages + // from is deleted at turn end. So the call must run in every mode, and it + // does no external harm in any of them: no agent-driven change reaches a + // remote without an operator approving the push, which is the property + // `readonly` actually promises, kept here by the approval rather than by + // refusing a harmless local stage. `EffectGroup::Publish` is the label the + // operator's approval card carries; `PerCall` because a standing "publish + // whenever" is exactly the grant the `Standing` field refuses to describe, + // and every push already parks as its own approval regardless. + d("repo_publish", EffectGroup::Publish, Reach::Nothing), ]; /// A per-call declaration — the default. `const fn` so [`DECLARED`] stays a diff --git a/src/runtime/cycle.rs b/src/runtime/cycle.rs index 330bacb80..7daddae88 100644 --- a/src/runtime/cycle.rs +++ b/src/runtime/cycle.rs @@ -62,6 +62,17 @@ const HISTORY_LIMIT: usize = 32; /// park cards that silently do nothing when approved. pub(crate) const EMAIL_SEND_KIND: &str = "email.send"; +/// The effect kind a `repo_publish` approval performs (issue #735) — the +/// host-side push to the real remote. +/// +/// `pub(crate)` and defined here, in the always-compiled runtime, rather than in +/// the `openhuman`-gated `harness::repo` that builds it: `perform_effect` below +/// matches on it in the default build, where `crate::harness` does not exist. The +/// tool references it through `crate::runtime::cycle::REPO_PUBLISH_EFFECT`, the +/// same shape `workflows::delivery` uses for [`EMAIL_SEND_KIND`], so the producer +/// and this consumer key off one literal. +pub(crate) const REPO_PUBLISH_EFFECT: &str = "repo.publish"; + /// The `error` the terminality backstop stamps on an attempt row whose cycle /// ended without settling it (issue #242) — a brain that ignored the dispatch, /// not a brain that failed at it. @@ -1231,9 +1242,136 @@ async fn perform_effect(rt: &CompanyRuntime, effect: &Effect) -> Result<()> { if effect.kind == crate::runtime::WORKFLOW_APPROVE_KIND { crate::runtime::workflow_resume::resume_from_effect(rt, effect).await?; } + // Issue #735: an approved `repo_publish`. `execute` already staged the agent's + // commits onto the mirror's `oc//` ref (the reversible half); + // this is the irreversible half — the host-side push to the real remote, done + // only now that the operator has approved. At-most-once comes free from the + // `approval:` key the caller holds; a denied or expired approval never + // reaches here, which is exactly what leaves the remote untouched. + if effect.kind == REPO_PUBLISH_EFFECT { + let repo = effect + .payload + .get("repo") + .and_then(|v| v.as_str()) + .unwrap_or_default(); + let branch = effect + .payload + .get("branch") + .and_then(|v| v.as_str()) + .unwrap_or_default(); + // The exact commit the approval was bound to at stage time. Pushing this + // SHA — not whatever the mirror's branch ref points at now — is what stops + // a second publish on the same task from riding in on this approval. + let head = effect + .payload + .get("head") + .and_then(|v| v.as_str()) + .unwrap_or_default(); + let task = effect + .payload + .get("task") + .and_then(|v| v.as_str()) + .unwrap_or_default(); + let agent = effect + .payload + .get("agent") + .and_then(|v| v.as_str()) + .unwrap_or_default(); + let message = effect + .payload + .get("message") + .and_then(|v| v.as_str()) + .unwrap_or_default(); + let Some(repos) = rt.repos() else { + return Err(crate::error::OpenCompanyError::Unimplemented( + "a repository publish was approved but this host has no repository manager \ + configured to perform it", + )); + }; + // The push is the irreversible half: its failure fails the effect. + repos.push_published(repo, branch, head).await?; + + // Issue #736: open a pull request for the pushed branch, best-effort. The + // push has landed, so a PR failure must NOT fail the effect — the branch + // is on the remote regardless. It is reported instead: the operator is + // told, on the task itself, that the branch is up but the PR did not open. + let title = repo_publish_pr_title(message); + let body = repo_publish_pr_body(agent, task, message); + match repos.open_pull_request(repo, branch, &title, &body).await { + Ok(pr) => tracing::info!( + number = pr.number, + url = %pr.html_url, + branch, + "[repo] opened a pull request for the published branch" + ), + Err(err) => { + tracing::warn!( + branch, + "[repo] pushed the branch but could not open a pull request: {err}" + ); + if !task.is_empty() { + let note = format!( + "Published `{branch}` to the remote, but the pull request could not be \ + opened: {err}. The branch is on the remote — open a PR from it by hand, \ + or approve another publish to retry." + ); + if let Err(e) = rt + .events + .append( + &rt.id, + CompanyEvent::TaskDiscussionPosted { + task_id: task.to_string(), + text: note, + by: None, + }, + ) + .await + { + tracing::warn!( + "[repo] could not record the pull-request failure on the task: {e}" + ); + } + } + } + } + } Ok(()) } +/// The title of the pull request a `repo_publish` opens (issue #736): the first +/// line of the agent's message, bounded, or a plain fallback when it said +/// nothing. +fn repo_publish_pr_title(message: &str) -> String { + let first = message.trim().lines().next().unwrap_or("").trim(); + if first.is_empty() { + "Published by an OpenCompany agent".to_string() + } else { + first.chars().take(72).collect() + } +} + +/// The body of that pull request (issue #736): the agent's message, then task +/// and agent linkage so an operator landing on the PR can get back to the card +/// and the seat that produced it. +fn repo_publish_pr_body(agent: &str, task: &str, message: &str) -> String { + let mut body = String::new(); + let message = message.trim(); + if !message.is_empty() { + body.push_str(message); + body.push_str("\n\n"); + } + body.push_str("---\n"); + body.push_str("Opened host-side by an OpenCompany agent"); + if !agent.is_empty() { + body.push_str(&format!(" (`{agent}`)")); + } + if !task.is_empty() { + body.push_str(&format!(" for task `{task}`")); + } + body.push('.'); + body +} + /// Sends an `email.send` effect via the company's own outbound-mail handle /// and records the send to the sender's own inbox (so the console shows /// outbound mail alongside inbound). diff --git a/src/runtime/repo_manager.rs b/src/runtime/repo_manager.rs index f136826f4..89e24b249 100644 --- a/src/runtime/repo_manager.rs +++ b/src/runtime/repo_manager.rs @@ -83,7 +83,8 @@ use crate::ports::types::{CompanyId, SecretValue}; #[cfg(feature = "github")] pub use github::HttpRepoHost; pub use types::{ - BindRequest, PullRequestView, RepoBinding, RepoCoordinates, RepoHost, RepoMeta, TokenKind, + BindRequest, PullRequestRef, PullRequestView, RepoBinding, RepoCoordinates, RepoHost, RepoMeta, + TokenKind, }; use types::{RepoIndex, classify_token, fingerprint, parse_repo_url}; @@ -794,6 +795,194 @@ impl RepoManager { ))) } + // -- publish (issue #735) ------------------------------------------------ + // + // The write tier's one git write direction. Split in two on purpose, because + // the approval gate sits between them: + // + // * `stage_publish` runs while the agent's task-scoped checkout still + // exists — it FETCHES the agent's committed HEAD out of that checkout + // and into the mirror on a host-owned `oc//` ref. This is + // the step that must survive the checkout being purged at turn end, so it + // happens before anything parks. It is a *fetch*, so — exactly as + // #245's contract depends on — it never invokes the mirror's + // `receive-pack` and the `pre-receive` push-refusal hook is never + // consulted. The agent still holds no credentialed remote and never + // pushes; the host moves the objects. + // * `push_published` runs only after the operator approves — it pushes that + // already-staged ref from the mirror to the real remote, host-side, where + // the credential lives. A denied or expired approval simply never calls + // it, so the remote is untouched. + // + // Every structural refusal lives here, not in a tool description: the branch + // is generated host-side and re-validated on the way out, so a prompt-injected + // agent asking for `--force origin main` has no code path to reach one. + + /// The one branch an agent's work is ever pushed to, generated host-side. + /// + /// `oc//`, and nothing else: the namespace is owned by us, the + /// company is this manager's own id (never agent-supplied), and the task + /// segment is validated to a single safe path component before it is placed + /// in a ref. Anything an agent could influence is bounded to that one + /// segment, and the whole ref is then run through the same validator the bind + /// path uses. + fn publish_branch(&self, task: &str) -> Result { + let segment = validate_task_segment(task)?; + let branch = format!("oc/{}/{}", self.company.as_ref() as &str, segment); + // Belt-and-braces: the company id is a validated slug and `segment` is + // already checked, but the whole ref goes through the shared validator so + // there is exactly one shape rule for every branch this crate creates. + validate_ref(&branch)?; + Ok(branch) + } + + /// Refuses any branch that is not a publish branch this manager owns. + /// + /// Defense in depth: `stage_publish` only ever constructs a good branch via + /// [`publish_branch`], but the push is a separate call that could be reached + /// with a hand-built argument, so it re-checks rather than trusts. This is + /// the single place "never a ref outside `oc/`, never the default branch" + /// is enforced — the default branch (and every other real branch) lives + /// outside the `oc//` namespace by construction, so the prefix check + /// *is* the default-branch refusal. + fn assert_publish_branch(&self, branch: &str) -> Result<()> { + let prefix = format!("oc/{}/", self.company.as_ref() as &str); + if !branch.starts_with(&prefix) || branch.len() <= prefix.len() { + return Err(OpenCompanyError::InvalidRequest(format!( + "{branch:?} is not a publish branch — the write tier only ever pushes to \ + {prefix}, never to the default branch or any ref outside that namespace" + ))); + } + validate_ref(branch)?; + Ok(()) + } + + /// Stages the agent's committed work onto the mirror's `oc//` + /// ref, host-side, and returns that branch name. + /// + /// `checkout` is the agent's task-scoped clone (`workspace/repos/`), + /// resolved host-side from the workspace and the binding key — never a path + /// the agent typed. Its committed `HEAD` is fetched into the mirror over the + /// `file://` transport; a fetch writes the ref without ever running + /// `receive-pack`, so the mirror's unconditional push-refusal hook (and + /// #245's contract test asserting it) is untouched. + pub async fn stage_publish( + &self, + key: &str, + checkout: &Path, + task: &str, + ) -> Result<(String, String)> { + // Ensures the repository is actually bound to this company before any git + // runs — an unbound key resolves to no mirror and no credential. + let _binding = self.get(key).await?; + let branch = self.publish_branch(task)?; + + let mirror = self.mirror_path(key); + if !mirror.is_dir() { + return Err(OpenCompanyError::NotFound(format!( + "the mirror for {key} is missing from the cache — revoke and rebind" + ))); + } + if !checkout.is_dir() { + return Err(OpenCompanyError::InvalidRequest( + "there is nothing to publish: check out the repository and commit your work \ + before publishing" + .to_string(), + )); + } + + // Fetch the checkout's committed HEAD into the mirror as the host-owned + // branch. No credential: the source is a local `file://` path. The `+` + // forces only the *local* (mirror) ref, so re-staging a publish is clean; + // it says nothing about the remote, which the push below never forces. + let url = file_url(checkout); + let refspec = format!("+HEAD:refs/heads/{branch}"); + let out = git::run( + &mirror, + &["fetch", "--quiet", "--no-tags", &url, &refspec], + None, + None, + ) + .await?; + if !out.ok { + return Err(OpenCompanyError::Store(format!( + "could not stage the publish: {}", + first_line(&out.stderr) + ))); + } + + // The exact commit just staged. The approval is bound to this SHA and the + // push sends this SHA, so a second `repo_publish` on the same task — which + // force-updates the branch ref above — cannot change what an earlier + // approval publishes. + let head = git::run( + &mirror, + &["rev-parse", &format!("refs/heads/{branch}")], + None, + None, + ) + .await? + .require("reading the staged commit")?; + Ok((branch, head)) + } + + /// Pushes an already-staged `oc//` branch from the mirror to + /// the real remote, host-side, where the credential lives. + /// + /// Called only after the operator approves — a denied or expired approval + /// never reaches here, so the remote stays untouched. **Never a force push + /// and never a `+` refspec:** a non-fast-forward is reported as a failure, + /// not overwritten. The branch is re-validated as a publish branch this + /// manager owns, and `head` is the exact commit the approval was bound to, + /// before a single byte leaves the container. + pub async fn push_published(&self, key: &str, branch: &str, head: &str) -> Result<()> { + let binding = self.get(key).await?; + self.assert_publish_branch(branch)?; + // The commit the operator approved, as a bare object id. Validated so it + // can name exactly one object and nothing but one — a value carrying a + // space or a `:` would otherwise reshape the refspec below. + if head.len() != 40 || !head.bytes().all(|b| b.is_ascii_hexdigit()) { + return Err(OpenCompanyError::InvalidRequest(format!( + "{head:?} is not a commit id" + ))); + } + + let token = self.token_for(&binding).await?.ok_or_else(|| { + OpenCompanyError::InvalidRequest(format!( + "no credential is stored for {key}; rebind it before publishing" + )) + })?; + + let mirror = self.mirror_path(key); + if !mirror.is_dir() { + return Err(OpenCompanyError::NotFound(format!( + "the mirror for {key} is missing from the cache — revoke and rebind" + ))); + } + + // Push the exact approved commit to the branch. `:refs/heads/` + // sends that object no matter where the mirror's branch ref points now, so + // a second publish that re-staged this task's branch cannot substitute a + // different commit into this approval. Still no leading `+` and no + // `--force`: a non-fast-forward on the remote is refused, not clobbered. + let refspec = format!("{head}:refs/heads/{branch}"); + let askpass = git::AskpassDir::create(&self.root)?; + let out = git::run( + &mirror, + &["push", "--quiet", "origin", &refspec], + Some(&token), + Some(&askpass), + ) + .await?; + if !out.ok { + return Err(OpenCompanyError::Store(format!( + "could not push {branch}: {}", + first_line(&out.stderr) + ))); + } + Ok(()) + } + // -- pull requests ------------------------------------------------------- /// A pull request's metadata and unified diff, fetched host-side. @@ -821,6 +1010,58 @@ impl RepoManager { host.pull_request(&coords, number, &token).await } + /// Opens a pull request from an already-published `oc//` branch + /// into the repository's default branch, host-side (issue #736). + /// + /// Degrades honestly, the same shape [`pull_request`](Self::pull_request) + /// uses: with no forge client wired this is `Unimplemented` rather than a + /// silent no-op, so a caller can tell "the PR was not opened" from "the PR was + /// opened empty". The base is fetched fresh so the PR always targets what the + /// forge considers default now, and a read-only binding is refused here too — + /// PR creation rides the same push-capable credential the publish did, and no + /// agent ever reaches this: the client stays host-side. + pub async fn open_pull_request( + &self, + key: &str, + branch: &str, + title: &str, + body: &str, + ) -> Result { + let binding = self.get(key).await?; + let Some(host) = self.host.as_ref() else { + return Err(OpenCompanyError::Unimplemented( + "opening a pull request needs a forge client; rebuild with the `github` feature", + )); + }; + if binding.can_push != Some(true) { + return Err(OpenCompanyError::InvalidRequest(format!( + "{} is bound with a credential that cannot push, so no pull request can be opened", + binding.url + ))); + } + let Some(token) = self.token_for(&binding).await? else { + return Err(OpenCompanyError::InvalidRequest(format!( + "{} is bound without a credential, so no pull request can be opened", + binding.url + ))); + }; + let coords = RepoCoordinates { + owner: binding.owner.clone(), + repo: binding.repo.clone(), + }; + let base = host.repo_meta(&coords, &token).await?.default_branch; + // A published branch is `oc//`, so it is never the default + // branch — but assert it rather than trust it, because GitHub rejects a + // pull request from a branch into itself with an opaque 422. + if branch == base { + return Err(OpenCompanyError::InvalidRequest(format!( + "refusing to open a pull request from {base:?} into itself" + ))); + } + host.create_pull_request(&coords, &token, branch, &base, title, body) + .await + } + /// Binds a repository from a URL this surface does not otherwise accept — /// a local `file://` fixture — with no credential. /// @@ -979,6 +1220,58 @@ pub(crate) fn validate_ref(raw: &str) -> Result { Ok(name.to_string()) } +/// Validates the task-id component of a publish branch to a single safe path +/// segment (issue #735). +/// +/// The task id is the one part of `oc//` that originates outside +/// this manager, so it is held tighter than a whole ref: no `/` — it may not add +/// path segments — no `..`, no leading `-`, and only the same character class the +/// ref validator accepts. `` is this manager's own validated id. +fn validate_task_segment(task: &str) -> Result { + let name = task.trim(); + let refuse = |why: &str| { + Err(OpenCompanyError::InvalidRequest(format!( + "{name:?} is not a usable task id for a publish branch — {why}" + ))) + }; + if name.is_empty() { + return refuse("it is empty"); + } + if name.len() > 128 { + return refuse("it is too long"); + } + if name.starts_with('-') { + return refuse("a leading '-' would be read as a command-line option"); + } + if name.contains('/') || name.contains("..") { + return refuse("it must be a single path segment"); + } + if !name + .chars() + .all(|c| c.is_ascii_alphanumeric() || matches!(c, '-' | '_' | '.')) + { + return refuse("only letters, digits, '-', '_' and '.' are accepted"); + } + Ok(name.to_string()) +} + +/// A `file://` URL for a local repository path — the transport host-side fetches +/// between the mirror and a checkout use, matching the checkout tier. +fn file_url(path: &Path) -> String { + format!("file://{}", path.display()) +} + +/// git's first non-empty stderr line, for an error that names what went wrong +/// without pasting a whole transcript into an API response. +fn first_line(stderr: &str) -> String { + stderr + .lines() + .map(str::trim) + .find(|l| !l.is_empty()) + .unwrap_or("git wrote nothing to stderr") + .to_string() +} + /// The `pre-receive` hook every mirror carries: a receiving end that refuses /// every push, whatever it is called with. /// diff --git a/src/runtime/repo_manager/github.rs b/src/runtime/repo_manager/github.rs index e1915c985..d30c1c9e5 100644 --- a/src/runtime/repo_manager/github.rs +++ b/src/runtime/repo_manager/github.rs @@ -20,7 +20,7 @@ use async_trait::async_trait; -use super::types::{PullRequestView, RepoCoordinates, RepoHost, RepoMeta}; +use super::types::{PullRequestRef, PullRequestView, RepoCoordinates, RepoHost, RepoMeta}; use crate::Result; use crate::error::OpenCompanyError; @@ -125,6 +125,72 @@ impl HttpRepoHost { let bytes = read_capped(stream, MAX_DIFF_BYTES).await?; Ok(truncate_utf8(&bytes, MAX_DIFF_BYTES)) } + + /// Issues an authenticated POST of a JSON body and returns the response body + /// (issue #736). + /// + /// Shares [`get`](Self::get)'s auth headers and its credential-error mapping + /// — 401/403/404 all become the same `InvalidRequest` naming the token, since + /// GitHub answers 404 for a repository a fine-grained token was not granted. + /// A non-success status carries GitHub's own explanation (a create failure — + /// "a pull request already exists", a protected base — is explained in the + /// body), trimmed to its first line so an API response never pastes a wall of + /// JSON into ours. The response of a create is a single small object, so it + /// is read whole rather than through the diff cap. + async fn post(&self, url: &str, token: &str, body: &serde_json::Value) -> Result { + let response = self + .http + .post(url) + .header("Authorization", format!("Bearer {token}")) + .header("Accept", "application/vnd.github+json") + .header("X-GitHub-Api-Version", "2022-11-28") + .json(body) + .send() + .await + .map_err(|e| OpenCompanyError::Store(format!("could not reach the GitHub API: {e}")))?; + let status = response.status(); + if status == reqwest::StatusCode::UNAUTHORIZED + || status == reqwest::StatusCode::FORBIDDEN + || status == reqwest::StatusCode::NOT_FOUND + { + return Err(OpenCompanyError::InvalidRequest( + "GitHub refused that credential for this repository. A fine-grained token \ + answers 404 for a repository it was not granted, so check that the token \ + lists this repository and has write access to Contents and Pull requests — \ + and that it has not expired." + .to_string(), + )); + } + if !status.is_success() { + let detail = response.text().await.unwrap_or_default(); + let detail = github_message(&detail); + return Err(OpenCompanyError::Store(format!( + "the GitHub API answered {status}{}", + if detail.is_empty() { + String::new() + } else { + format!(": {detail}") + } + ))); + } + response + .text() + .await + .map_err(|e| OpenCompanyError::Store(format!("reading the GitHub response: {e}"))) + } +} + +/// The human `message` field GitHub puts on an error response, if any — the one +/// sentence worth surfacing out of a JSON error body. +fn github_message(body: &str) -> String { + serde_json::from_str::(body) + .ok() + .and_then(|v| { + v.get("message") + .and_then(|m| m.as_str()) + .map(str::to_string) + }) + .unwrap_or_default() } /// Collects a byte stream until it holds more than `limit`, then stops. @@ -229,6 +295,37 @@ impl RepoHost for HttpRepoHost { diff, }) } + + async fn create_pull_request( + &self, + coords: &RepoCoordinates, + token: &str, + head: &str, + base: &str, + title: &str, + body: &str, + ) -> Result { + let url = format!("{}/repos/{}/{}/pulls", self.base, coords.owner, coords.repo); + let payload = serde_json::json!({ + "title": title, + "head": head, + "base": base, + "body": body, + }); + let response = self.post(&url, token, &payload).await?; + let json: serde_json::Value = serde_json::from_str(&response)?; + let number = json.get("number").and_then(|v| v.as_u64()).ok_or_else(|| { + OpenCompanyError::Store( + "GitHub accepted the pull request but its response named no number".to_string(), + ) + })?; + let html_url = json + .get("html_url") + .and_then(|v| v.as_str()) + .unwrap_or_default() + .to_string(); + Ok(PullRequestRef { number, html_url }) + } } #[cfg(test)] diff --git a/src/runtime/repo_manager/test.rs b/src/runtime/repo_manager/test.rs index 7b2e5e01f..440c3a7a7 100644 --- a/src/runtime/repo_manager/test.rs +++ b/src/runtime/repo_manager/test.rs @@ -74,11 +74,25 @@ impl MemSecrets { } } +/// One `create_pull_request` call, recorded so a test can assert what was sent. +#[derive(Clone, Debug)] +struct CreatedPr { + head: String, + base: String, + title: String, + body: String, +} + /// A forge that answers from a script, and records the token it was handed. struct FakeHost { meta: RepoMeta, seen_tokens: StdMutex>, fail: bool, + /// When set, `create_pull_request` errors — for the honest-degradation test + /// where a push succeeds but the PR does not open (issue #736). + fail_pr: bool, + /// Every `create_pull_request` call, in order. + created_prs: StdMutex>, } impl FakeHost { @@ -93,6 +107,8 @@ impl FakeHost { }, seen_tokens: StdMutex::new(Vec::new()), fail: false, + fail_pr: false, + created_prs: StdMutex::new(Vec::new()), } } @@ -109,6 +125,12 @@ impl FakeHost { self.meta.can_push = true; self } + + /// A forge that accepts a push but refuses to open the pull request. + fn failing_pr(mut self) -> Self { + self.fail_pr = true; + self + } } #[async_trait] @@ -137,6 +159,33 @@ impl RepoHost for FakeHost { diff: "--- a\n+++ b\n".into(), }) } + + async fn create_pull_request( + &self, + _coords: &RepoCoordinates, + token: &str, + head: &str, + base: &str, + title: &str, + body: &str, + ) -> Result { + self.seen_tokens.lock().unwrap().push(token.to_string()); + if self.fail_pr { + return Err(OpenCompanyError::Store( + "the forge refused the pull request".into(), + )); + } + self.created_prs.lock().unwrap().push(CreatedPr { + head: head.to_string(), + base: base.to_string(), + title: title.to_string(), + body: body.to_string(), + }); + Ok(PullRequestRef { + number: 42, + html_url: "https://github.com/acme/fixture/pull/42".into(), + }) + } } /// A scratch directory removed when the test ends. @@ -1083,6 +1132,360 @@ async fn a_known_capability_is_not_re_probed_on_every_fetch() { ); } +// -- publish (issue #735) ---------------------------------------------------- + +/// Clones `mirror` into `dest` and commits one file, returning the new HEAD SHA. +/// Stands in for an agent's task-scoped checkout with committed work — the thing +/// `stage_publish` fetches from. +fn checkout_with_commit( + scratch: &Scratch, + mirror: &Path, + dest: &Path, + file: &str, + body: &str, +) -> String { + git_at( + &scratch.0, + &[ + "clone", + "--quiet", + mirror.to_str().unwrap(), + dest.to_str().unwrap(), + ], + ); + for (k, v) in [ + ("user.email", "agent@acme.test"), + ("user.name", "Agent Seat"), + ("commit.gpgsign", "false"), + ] { + git_at(dest, &["config", k, v]); + } + std::fs::write(dest.join(file), body).unwrap(); + git_at(dest, &["add", file]); + git_at(dest, &["commit", "--quiet", "-m", "agent work"]); + git_at(dest, &["rev-parse", "HEAD"]) +} + +/// The happy path end to end: the agent's committed HEAD is staged onto the +/// host-owned `oc//` ref in the mirror, then pushed to the remote +/// as exactly that branch and commit. +#[tokio::test] +async fn a_publish_stages_the_agents_commit_and_pushes_the_namespaced_branch() { + let scratch = Scratch::new("publish"); + let url = fixture_remote(&scratch); + let (mgr, secrets) = manager(&scratch); + mgr.bind_local(&url, "fixture", vec!["main".into()]) + .await + .unwrap(); + secrets + .set( + &CompanyId::new("acme"), + &repo_token_key("fixture"), + SecretValue(SENTINEL.into()), + ) + .await + .unwrap(); + + let mirror = mgr.mirror_path("fixture"); + let checkout = scratch.join("checkout"); + let head = checkout_with_commit(&scratch, &mirror, &checkout, "FIX.md", "the fix\n"); + + // Stage: the agent's HEAD lands on the host-owned branch, host-side, and the + // exact staged commit is returned so the approval can be bound to it. + let (branch, staged_head) = mgr + .stage_publish("fixture", &checkout, "task-1") + .await + .unwrap(); + assert_eq!(branch, "oc/acme/task-1", "the branch is host-namespaced"); + assert_eq!(staged_head, head, "stage_publish returns the staged commit"); + let staged = git_at(&mirror, &["rev-parse", "refs/heads/oc/acme/task-1"]); + assert_eq!(staged, head, "the mirror ref points at the agent's commit"); + + // Push: the remote receives exactly that branch and commit. + mgr.push_published("fixture", &branch, &staged_head) + .await + .unwrap(); + let bare = scratch.join("origin.git"); + let pushed = git_at(&bare, &["rev-parse", "refs/heads/oc/acme/task-1"]); + assert_eq!(pushed, head, "the remote branch is the agent's commit"); +} + +/// The task id is the one part of the branch that comes from outside the +/// manager, so an unsafe one is refused before any git runs — no `/` (it may not +/// add path segments), no `..`, no leading `-`, no odd characters. +#[tokio::test] +async fn a_publish_task_id_that_is_not_a_safe_segment_is_refused() { + let scratch = Scratch::new("bad-task"); + let url = fixture_remote(&scratch); + let (mgr, _) = manager(&scratch); + mgr.bind_local(&url, "fixture", vec!["main".into()]) + .await + .unwrap(); + + // The task is validated before the checkout is even looked at, so the path + // does not need to exist for this to refuse. + let checkout = scratch.join("unused"); + for bad in ["../evil", "a/b", "-rf", "", "with space", "has..dots"] { + let err = mgr + .stage_publish("fixture", &checkout, bad) + .await + .unwrap_err(); + assert!( + matches!(err, OpenCompanyError::InvalidRequest(_)), + "task {bad:?} should be refused: {err:?}" + ); + } +} + +/// The push refuses any branch that is not a publish branch this company owns — +/// the default branch, another company's namespace, the bare prefix, or a +/// traversal — enforced in `RepoManager`, not by the tool description. +#[tokio::test] +async fn a_push_to_anything_but_this_companys_namespace_is_refused() { + let scratch = Scratch::new("bad-push"); + let url = fixture_remote(&scratch); + let (mgr, secrets) = manager(&scratch); + mgr.bind_local(&url, "fixture", vec!["main".into()]) + .await + .unwrap(); + secrets + .set( + &CompanyId::new("acme"), + &repo_token_key("fixture"), + SecretValue(SENTINEL.into()), + ) + .await + .unwrap(); + + for bad in [ + "main", // the default branch + "oc/other/task-1", // a foreign company's namespace + "oc/acme/", // the bare prefix, no task + "oc/acme/../main", // a traversal out of the namespace + "refs/heads/main", // a fully-qualified default ref + ] { + // A valid-shaped commit id, so it is the branch that is refused — the + // branch is re-validated before the head is even looked at. + let err = mgr + .push_published("fixture", bad, &"a".repeat(40)) + .await + .unwrap_err(); + assert!( + matches!(err, OpenCompanyError::InvalidRequest(_)), + "branch {bad:?} should be refused: {err:?}" + ); + } +} + +/// The push is never a force push: a branch that already exists on the remote +/// and would not fast-forward is refused by the remote, leaving the earlier +/// commit in place, rather than being overwritten. +#[tokio::test] +async fn a_non_fast_forward_publish_is_refused_never_forced() { + let scratch = Scratch::new("no-force"); + let url = fixture_remote(&scratch); + let (mgr, secrets) = manager(&scratch); + mgr.bind_local(&url, "fixture", vec!["main".into()]) + .await + .unwrap(); + secrets + .set( + &CompanyId::new("acme"), + &repo_token_key("fixture"), + SecretValue(SENTINEL.into()), + ) + .await + .unwrap(); + let mirror = mgr.mirror_path("fixture"); + let bare = scratch.join("origin.git"); + + // First publish: commit A lands on the remote branch. + let c1 = scratch.join("checkout1"); + let head_a = checkout_with_commit(&scratch, &mirror, &c1, "A.md", "A\n"); + mgr.stage_publish("fixture", &c1, "task-1").await.unwrap(); + mgr.push_published("fixture", "oc/acme/task-1", &head_a) + .await + .unwrap(); + + // A divergent commit B (a sibling of A, not its descendant) staged onto the + // same branch and pushed. Since the push never forces, the remote refuses + // the non-fast-forward. + let c2 = scratch.join("checkout2"); + let head_b = checkout_with_commit(&scratch, &mirror, &c2, "B.md", "B\n"); + mgr.stage_publish("fixture", &c2, "task-1").await.unwrap(); + let err = mgr + .push_published("fixture", "oc/acme/task-1", &head_b) + .await + .unwrap_err(); + assert!( + matches!(err, OpenCompanyError::Store(_)), + "a non-fast-forward publish must be refused, not forced: {err:?}" + ); + + // And the remote still holds A — nothing was clobbered. + let remote = git_at(&bare, &["rev-parse", "refs/heads/oc/acme/task-1"]); + assert_eq!( + remote, head_a, + "the remote branch must still point at the first commit" + ); +} + +/// An approval is bound to the exact commit it was staged for (issue #735). A +/// second publish on the same task force-updates the mirror's branch ref, but +/// approving the first still publishes the first commit — the later re-stage +/// cannot ride in on the earlier approval. +#[tokio::test] +async fn a_publish_pushes_the_approved_commit_even_after_a_restage() { + let scratch = Scratch::new("bound-commit"); + let url = fixture_remote(&scratch); + let (mgr, secrets) = manager(&scratch); + mgr.bind_local(&url, "fixture", vec!["main".into()]) + .await + .unwrap(); + secrets + .set( + &CompanyId::new("acme"), + &repo_token_key("fixture"), + SecretValue(SENTINEL.into()), + ) + .await + .unwrap(); + let mirror = mgr.mirror_path("fixture"); + let bare = scratch.join("origin.git"); + + // First publish stages commit A and records its head. + let c1 = scratch.join("checkout1"); + checkout_with_commit(&scratch, &mirror, &c1, "A.md", "A\n"); + let (_branch, head_a) = mgr.stage_publish("fixture", &c1, "task-1").await.unwrap(); + + // Before it is approved, a second publish on the SAME task stages commit B, + // force-updating the mirror's branch ref to B. + let c2 = scratch.join("checkout2"); + let head_b = checkout_with_commit(&scratch, &mirror, &c2, "B.md", "B\n"); + mgr.stage_publish("fixture", &c2, "task-1").await.unwrap(); + assert_ne!(head_a, head_b); + assert_eq!( + git_at(&mirror, &["rev-parse", "refs/heads/oc/acme/task-1"]), + head_b, + "the mirror ref now points at the re-staged commit B" + ); + + // Approving the FIRST publish pushes A — the commit that approval was bound + // to — not whatever the branch ref points at now. + mgr.push_published("fixture", "oc/acme/task-1", &head_a) + .await + .unwrap(); + let remote = git_at(&bare, &["rev-parse", "refs/heads/oc/acme/task-1"]); + assert_eq!( + remote, head_a, + "the approved commit reached the remote, not the re-staged one" + ); +} + +// -- pull-request creation (issue #736) -------------------------------------- + +/// A manager with a push-capable binding: `bind_local` records no capability, so +/// a `pushable` host + a fetch heals it to `Some(true)`, the state +/// `open_pull_request` requires. +async fn pushable_bound(scratch: &Scratch, host: Arc) -> RepoManager { + let url = fixture_remote(scratch); + let (mgr, secrets) = manager(scratch); + let mgr = mgr.with_host(host); + mgr.bind_local(&url, "fixture", vec!["main".into()]) + .await + .unwrap(); + secrets + .set( + &CompanyId::new("acme"), + &repo_token_key("fixture"), + SecretValue(SENTINEL.into()), + ) + .await + .unwrap(); + // Heals can_push from None to Some(true) via the pushable host. + mgr.fetch("fixture", &[]).await.unwrap(); + mgr +} + +/// A PR is opened from the published branch into the repository's **default** +/// branch, carrying the title and body the caller built. +#[tokio::test] +async fn open_pull_request_targets_the_default_branch_with_the_given_body() { + let scratch = Scratch::new("open-pr"); + let host = Arc::new(FakeHost::new(1).pushable()); + let mgr = pushable_bound(&scratch, host.clone()).await; + + let pr = mgr + .open_pull_request("fixture", "oc/acme/card-1", "the fix", "body with #card-1") + .await + .unwrap(); + assert_eq!(pr.number, 42); + + let created = host.created_prs.lock().unwrap(); + assert_eq!(created.len(), 1); + assert_eq!(created[0].head, "oc/acme/card-1"); + assert_eq!( + created[0].base, "main", + "the base is the repository's default branch" + ); + assert_eq!(created[0].title, "the fix"); + assert!(created[0].body.contains("#card-1")); +} + +/// With no forge client wired, opening a PR is honestly unavailable rather than a +/// silent success — the shape `pull_request` already uses. +#[tokio::test] +async fn open_pull_request_without_a_forge_client_says_so() { + let scratch = Scratch::new("open-pr-unwired"); + let url = fixture_remote(&scratch); + let (mgr, _) = manager(&scratch); + mgr.bind_local(&url, "fixture", vec!["main".into()]) + .await + .unwrap(); + let err = mgr + .open_pull_request("fixture", "oc/acme/card-1", "t", "b") + .await + .unwrap_err(); + assert!(matches!(err, OpenCompanyError::Unimplemented(_)), "{err:?}"); +} + +/// A read-only binding is refused — PR creation rides the same push-capable +/// credential the publish did. +#[tokio::test] +async fn open_pull_request_refuses_a_read_only_binding() { + let scratch = Scratch::new("open-pr-readonly"); + let url = fixture_remote(&scratch); + let (mgr, _) = manager(&scratch); + // A read-only host: bind_local leaves can_push None, and nothing heals it. + let mgr = mgr.with_host(Arc::new(FakeHost::new(1))); + mgr.bind_local(&url, "fixture", vec!["main".into()]) + .await + .unwrap(); + let err = mgr + .open_pull_request("fixture", "oc/acme/card-1", "t", "b") + .await + .unwrap_err(); + assert!( + matches!(err, OpenCompanyError::InvalidRequest(_)), + "a read-only binding must be refused: {err:?}" + ); +} + +/// When the forge accepts the push but refuses the PR, `open_pull_request` +/// returns the error — the caller (`perform_effect`) is what keeps that from +/// failing the whole publish, reporting it on the task instead. +#[tokio::test] +async fn open_pull_request_surfaces_a_forge_refusal() { + let scratch = Scratch::new("open-pr-fail"); + let host = Arc::new(FakeHost::new(1).pushable().failing_pr()); + let mgr = pushable_bound(&scratch, host).await; + let err = mgr + .open_pull_request("fixture", "oc/acme/card-1", "t", "b") + .await + .unwrap_err(); + assert!(matches!(err, OpenCompanyError::Store(_)), "{err:?}"); +} + // -- index ------------------------------------------------------------------- #[tokio::test] diff --git a/src/runtime/repo_manager/types.rs b/src/runtime/repo_manager/types.rs index 1d9028a51..ce89e82ab 100644 --- a/src/runtime/repo_manager/types.rs +++ b/src/runtime/repo_manager/types.rs @@ -382,6 +382,17 @@ pub struct PullRequestView { pub diff: String, } +/// A newly opened pull request, as the create call answers (issue #736): the +/// two facts an operator needs — which PR, and where to open it. +#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct PullRequestRef { + /// The pull request number. + pub number: u64, + /// Its browser URL. + pub html_url: String, +} + /// The forge REST seam. /// /// Dependency-inverted for the same reason as the DNS resolver and the mail @@ -403,6 +414,19 @@ pub trait RepoHost: Send + Sync { number: u64, token: &str, ) -> Result; + + /// Opens a pull request from `head` into `base`, host-side, and returns the + /// created PR's number and browser URL (issue #736). The one *write* verb on + /// this seam — every other reaches the forge read-only. + async fn create_pull_request( + &self, + coords: &RepoCoordinates, + token: &str, + head: &str, + base: &str, + title: &str, + body: &str, + ) -> Result; } #[cfg(test)]