diff --git a/CLAUDE.md b/CLAUDE.md index 49cf893..1246944 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -183,6 +183,24 @@ read them à la carte. In-flight or recent context that hasn't earned a section yet. Prune freely. +- **Just landed:** `request_apply_commit` fetch fix. The old + `git_fetch_to_tag` built a refspec `:refs/tags/proposal/` + and ran `git fetch :...` — but `git fetch` resolves + the left side of a refspec as a remote *ref name*, and a bare + commit sha is not one ("couldn't find remote ref ..."). Fetching + by sha would need a full 40-hex sha plus + `uploadpack.allow*SHA1InWant` on the remote. Surfaced on the first + real `request_apply_commit` (the `gui` agent bootstrap — initial + `deployed/0` seeding uses a different path). Fix: `git_fetch_to_tag` + now resolves the sha LOCALLY against the proposed repo + (`git rev-parse ^{commit}`), fetches all of proposed's heads + into applied's object db (`+refs/heads/*:refs/remotes/proposal-src/*`), + then `git tag`s the resolved sha — all-local, no upload-pack + sha-want negotiation. Plus: `submit_apply_commit` now shape-checks + `commit_ref` is a 7-40 char hex sha (`validate_commit_ref`) and + rejects branch/tag names so the proposal always pins an immutable + commit. Tool description + `RequestApplyCommit` wire doc + + `docs/approvals.md` updated. 3 new tests in `manager_server::tests`. - **Just landed:** inbox batching unified into `recv(max?)`. No separate `recv_batch` tool — the existing `recv` tool grew an optional `max: u32` arg (default 1, server-side diff --git a/docs/approvals.md b/docs/approvals.md index 28e459d..d546022 100644 --- a/docs/approvals.md +++ b/docs/approvals.md @@ -12,9 +12,16 @@ happens after a decision lands. path, but `agent.nix` is the contract entry point) and commits with its own git identity. 2. Manager submits the commit sha via `request_apply_commit(agent, - commit_ref)`. + commit_ref)`. `commit_ref` must be a commit **sha** (7-40 hex + chars, short or full) — a branch or tag name is rejected so the + approval pins an immutable commit. 3. **hive-c0re immediately fetches that commit from the proposed - repo into the applied repo and tags it `proposal/`.** The + repo into the applied repo and tags it `proposal/`.** It + resolves the sha locally against the proposed repo, fetches all + of proposed's heads into applied's object db, then tags the + resolved commit — `git fetch :` can't fetch + by a bare sha (the left side of a refspec is a remote *ref + name*), so the resolution happens on hive-c0re's side. The approval row stores both the manager-supplied sha and the canonical hive-c0re-vouched sha. From here on the proposed repo is irrelevant for this approval — the manager can amend, diff --git a/hive-ag3nt/src/mcp.rs b/hive-ag3nt/src/mcp.rs index fc01432..568170f 100644 --- a/hive-ag3nt/src/mcp.rs +++ b/hive-ag3nt/src/mcp.rs @@ -738,7 +738,9 @@ pub struct RequestApplyCommitArgs { /// Agent whose config repo the commit lives in (use `"hm1nd"` for the /// manager's own config). pub agent: String, - /// Git sha (full or short) pointing at the proposed `agent.nix`. + /// Commit sha (full or short, 7-40 hex chars) in that agent's + /// proposed config repo. Must be a sha — a branch or tag name + /// (e.g. `main`) is rejected; the approval pins the exact commit. pub commit_ref: String, /// Optional description shown on the dashboard approval card so the /// operator knows what the change does without opening the diff. @@ -991,8 +993,10 @@ impl ManagerServer { #[tool( description = "Submit a config change for operator approval. Pass the agent name \ - (e.g. `alice` or `hm1nd` for the manager's own config) and a commit sha in that \ - agent's proposed config repo. On approval hive-c0re rebuilds the container." + (e.g. `alice` or `hm1nd` for the manager's own config) and a commit sha (7-40 hex \ + chars, full or short) in that agent's proposed config repo — a branch/tag name like \ + `main` is rejected, the approval pins the exact commit. On approval hive-c0re \ + rebuilds the container." )] async fn request_apply_commit( &self, diff --git a/hive-c0re/src/lifecycle.rs b/hive-c0re/src/lifecycle.rs index 345375a..648f273 100644 --- a/hive-c0re/src/lifecycle.rs +++ b/hive-c0re/src/lifecycle.rs @@ -559,17 +559,49 @@ async fn git(dir: &Path, args: &[&str]) -> Result<()> { Ok(()) } -/// Fetch `sha` from the `src` git repo into `dst` and pin it as -/// `refs/tags/`. Used at `request_apply_commit` time so hive-c0re -/// captures an immutable handle on the manager's commit; subsequent -/// amendments / force-pushes in `src` no longer affect what gets -/// built. Returns the resolved sha (which equals `sha` on success -/// but normalised — short shas get expanded). +/// Fetch the commit `sha` from the `src` git repo into `dst` and pin +/// it as `refs/tags/`. Used at `request_apply_commit` time so +/// hive-c0re captures an immutable handle on the manager's commit; +/// subsequent amendments / force-pushes in `src` no longer affect +/// what gets built. Returns the resolved full sha. +/// +/// `sha` must be a commit sha (short or full) — the caller +/// (`submit_apply_commit`) shape-checks it first. We resolve it +/// LOCALLY against `src` rather than asking the remote to resolve +/// it: `git fetch :` treats the left side as a +/// remote *ref name*, and a bare sha is not one ("couldn't find +/// remote ref ..."). Fetching by sha would need a full 40-hex sha +/// plus `uploadpack.allow*SHA1InWant` on the remote, which the +/// proposed repos don't set. hive-c0re has direct read access to +/// `src`, so a local `rev-parse` + a branch-glob fetch sidesteps +/// the whole sha-want negotiation. pub async fn git_fetch_to_tag(dst: &Path, src: &Path, sha: &str, tag: &str) -> Result { let src_str = src.display().to_string(); - let refspec = format!("{sha}:refs/tags/{tag}"); - git(dst, &["fetch", "--no-tags", &src_str, &refspec]).await?; - git_rev_parse(dst, &format!("refs/tags/{tag}")).await + // Resolve the (short-or-full) sha to a full sha against the + // source repo. The `^{commit}` peel + non-zero exit on a missing + // object means a typo'd / stale sha fails loudly right here. + let full = git_rev_parse(src, &format!("{sha}^{{commit}}")) + .await + .with_context(|| format!("commit '{sha}' not found in proposed repo {src_str}"))?; + // Bring src's objects into dst. Fetching every head pulls the + // wanted commit's history (always reachable from a branch in the + // manager's flow) into dst's object db without sha-want. + git( + dst, + &[ + "fetch", + "--no-tags", + &src_str, + "+refs/heads/*:refs/remotes/proposal-src/*", + ], + ) + .await?; + // Pin the exact commit as the proposal tag. The objects are now + // local so this resolves without touching the remote. + git(dst, &["tag", tag, &full]).await.with_context(|| { + format!("tag {tag} at {full}: commit not reachable from any branch in proposed repo") + })?; + Ok(full) } /// Resolve `refname` (a tag, branch, or sha) in `dir` to its full sha. diff --git a/hive-c0re/src/manager_server.rs b/hive-c0re/src/manager_server.rs index 5228c21..8417e0c 100644 --- a/hive-c0re/src/manager_server.rs +++ b/hive-c0re/src/manager_server.rs @@ -392,6 +392,25 @@ async fn dispatch(req: &ManagerRequest, coord: &Arc) -> ManagerResp } } +/// `request_apply_commit` takes a commit SHA only — not a branch or +/// tag name. A branch is mutable; pinning the proposal to a concrete +/// sha keeps "what the manager asked to deploy" unambiguous and means +/// the `proposal/` tag is a faithful record of the request. +/// Accepts a 7..=40 char hex string (short or full sha); the exact +/// commit is resolved + existence-checked against the proposed repo +/// later in `lifecycle::git_fetch_to_tag`. +fn validate_commit_ref(commit_ref: &str) -> Result<()> { + let n = commit_ref.len(); + let hex = commit_ref.chars().all(|c| c.is_ascii_hexdigit()); + if !(7..=40).contains(&n) || !hex { + anyhow::bail!( + "commit_ref '{commit_ref}' is not a commit sha — request_apply_commit \ + takes a 7-40 char hex sha, not a branch or tag name" + ); + } + Ok(()) +} + /// Submit-time half of the apply flow: queue the approval row, then /// fetch the manager's commit from the proposed repo into applied and /// pin it as `refs/tags/proposal/`. From this point on the manager @@ -409,6 +428,7 @@ async fn submit_apply_commit( commit_ref: &str, description: Option<&str>, ) -> anyhow::Result<(i64, String)> { + validate_commit_ref(commit_ref)?; let proposed_dir = crate::coordinator::Coordinator::agent_proposed_dir(agent); let applied_dir = crate::coordinator::Coordinator::agent_applied_dir(agent); if !proposed_dir.exists() { @@ -518,3 +538,33 @@ pub fn spawn_question_watchdog(coord: &Arc, id: i64, ttl_secs: u64) } }); } + +#[cfg(test)] +mod tests { + use super::validate_commit_ref; + + #[test] + fn accepts_short_and_full_sha() { + assert!(validate_commit_ref("e194f78").is_ok()); + assert!(validate_commit_ref("e194f7812ab").is_ok()); + assert!(validate_commit_ref(&"a".repeat(40)).is_ok()); + // Uppercase hex resolves fine through `git rev-parse`. + assert!(validate_commit_ref("E194F78").is_ok()); + } + + #[test] + fn rejects_branch_and_tag_names() { + // The exact bug class this guard exists for. + assert!(validate_commit_ref("main").is_err()); + assert!(validate_commit_ref("HEAD").is_err()); + assert!(validate_commit_ref("deployed/0").is_err()); + assert!(validate_commit_ref("feature-branch").is_err()); + } + + #[test] + fn rejects_too_short_too_long_and_empty() { + assert!(validate_commit_ref("").is_err()); + assert!(validate_commit_ref("abc123").is_err()); // 6 chars + assert!(validate_commit_ref(&"a".repeat(41)).is_err()); + } +} diff --git a/hive-sh4re/src/lib.rs b/hive-sh4re/src/lib.rs index 79aa9a3..435b62d 100644 --- a/hive-sh4re/src/lib.rs +++ b/hive-sh4re/src/lib.rs @@ -652,9 +652,11 @@ pub enum ManagerRequest { Update { name: String, }, - /// Submit a config commit for the user to approve. `commit_ref` is opaque - /// to the host (typically a git sha pointing into the agent's config repo). - /// On approval the host applies the change via `nixos-container update`. + /// Submit a config commit for the user to approve. `commit_ref` must + /// be a commit sha (7-40 hex chars, short or full) in the agent's + /// proposed config repo — a branch or tag name is rejected so the + /// approval pins an immutable commit. On approval the host applies + /// the change via `nixos-container update`. RequestApplyCommit { agent: String, commit_ref: String,