fix: request_apply_commit resolves sha locally + rejects non-sha refs
This commit is contained in:
parent
5d27ae3048
commit
f8795dc029
6 changed files with 130 additions and 17 deletions
18
CLAUDE.md
18
CLAUDE.md
|
|
@ -183,6 +183,24 @@ read them à la carte.
|
||||||
In-flight or recent context that hasn't earned a section yet.
|
In-flight or recent context that hasn't earned a section yet.
|
||||||
Prune freely.
|
Prune freely.
|
||||||
|
|
||||||
|
- **Just landed:** `request_apply_commit` fetch fix. The old
|
||||||
|
`git_fetch_to_tag` built a refspec `<sha>:refs/tags/proposal/<id>`
|
||||||
|
and ran `git fetch <proposed> <sha>:...` — 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 <sha>^{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?)`.
|
- **Just landed:** inbox batching unified into `recv(max?)`.
|
||||||
No separate `recv_batch` tool — the existing `recv` tool
|
No separate `recv_batch` tool — the existing `recv` tool
|
||||||
grew an optional `max: u32` arg (default 1, server-side
|
grew an optional `max: u32` arg (default 1, server-side
|
||||||
|
|
|
||||||
|
|
@ -12,9 +12,16 @@ happens after a decision lands.
|
||||||
path, but `agent.nix` is the contract entry point) and commits
|
path, but `agent.nix` is the contract entry point) and commits
|
||||||
with its own git identity.
|
with its own git identity.
|
||||||
2. Manager submits the commit sha via `request_apply_commit(agent,
|
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
|
3. **hive-c0re immediately fetches that commit from the proposed
|
||||||
repo into the applied repo and tags it `proposal/<id>`.** The
|
repo into the applied repo and tags it `proposal/<id>`.** 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 <remote> <sha>:<dst>` 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
|
approval row stores both the manager-supplied sha and the
|
||||||
canonical hive-c0re-vouched sha. From here on the proposed
|
canonical hive-c0re-vouched sha. From here on the proposed
|
||||||
repo is irrelevant for this approval — the manager can amend,
|
repo is irrelevant for this approval — the manager can amend,
|
||||||
|
|
|
||||||
|
|
@ -738,7 +738,9 @@ pub struct RequestApplyCommitArgs {
|
||||||
/// Agent whose config repo the commit lives in (use `"hm1nd"` for the
|
/// Agent whose config repo the commit lives in (use `"hm1nd"` for the
|
||||||
/// manager's own config).
|
/// manager's own config).
|
||||||
pub agent: String,
|
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,
|
pub commit_ref: String,
|
||||||
/// Optional description shown on the dashboard approval card so the
|
/// Optional description shown on the dashboard approval card so the
|
||||||
/// operator knows what the change does without opening the diff.
|
/// operator knows what the change does without opening the diff.
|
||||||
|
|
@ -991,8 +993,10 @@ impl ManagerServer {
|
||||||
|
|
||||||
#[tool(
|
#[tool(
|
||||||
description = "Submit a config change for operator approval. Pass the agent name \
|
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 \
|
(e.g. `alice` or `hm1nd` for the manager's own config) and a commit sha (7-40 hex \
|
||||||
agent's proposed config repo. On approval hive-c0re rebuilds the container."
|
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(
|
async fn request_apply_commit(
|
||||||
&self,
|
&self,
|
||||||
|
|
|
||||||
|
|
@ -559,17 +559,49 @@ async fn git(dir: &Path, args: &[&str]) -> Result<()> {
|
||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Fetch `sha` from the `src` git repo into `dst` and pin it as
|
/// Fetch the commit `sha` from the `src` git repo into `dst` and pin
|
||||||
/// `refs/tags/<tag>`. Used at `request_apply_commit` time so hive-c0re
|
/// it as `refs/tags/<tag>`. Used at `request_apply_commit` time so
|
||||||
/// captures an immutable handle on the manager's commit; subsequent
|
/// hive-c0re captures an immutable handle on the manager's commit;
|
||||||
/// amendments / force-pushes in `src` no longer affect what gets
|
/// subsequent amendments / force-pushes in `src` no longer affect
|
||||||
/// built. Returns the resolved sha (which equals `sha` on success
|
/// what gets built. Returns the resolved full sha.
|
||||||
/// but normalised — short shas get expanded).
|
///
|
||||||
|
/// `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 <remote> <sha>:<dst>` 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<String> {
|
pub async fn git_fetch_to_tag(dst: &Path, src: &Path, sha: &str, tag: &str) -> Result<String> {
|
||||||
let src_str = src.display().to_string();
|
let src_str = src.display().to_string();
|
||||||
let refspec = format!("{sha}:refs/tags/{tag}");
|
// Resolve the (short-or-full) sha to a full sha against the
|
||||||
git(dst, &["fetch", "--no-tags", &src_str, &refspec]).await?;
|
// source repo. The `^{commit}` peel + non-zero exit on a missing
|
||||||
git_rev_parse(dst, &format!("refs/tags/{tag}")).await
|
// 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.
|
/// Resolve `refname` (a tag, branch, or sha) in `dir` to its full sha.
|
||||||
|
|
|
||||||
|
|
@ -392,6 +392,25 @@ async fn dispatch(req: &ManagerRequest, coord: &Arc<Coordinator>) -> 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/<id>` 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
|
/// Submit-time half of the apply flow: queue the approval row, then
|
||||||
/// fetch the manager's commit from the proposed repo into applied and
|
/// fetch the manager's commit from the proposed repo into applied and
|
||||||
/// pin it as `refs/tags/proposal/<id>`. From this point on the manager
|
/// pin it as `refs/tags/proposal/<id>`. From this point on the manager
|
||||||
|
|
@ -409,6 +428,7 @@ async fn submit_apply_commit(
|
||||||
commit_ref: &str,
|
commit_ref: &str,
|
||||||
description: Option<&str>,
|
description: Option<&str>,
|
||||||
) -> anyhow::Result<(i64, String)> {
|
) -> anyhow::Result<(i64, String)> {
|
||||||
|
validate_commit_ref(commit_ref)?;
|
||||||
let proposed_dir = crate::coordinator::Coordinator::agent_proposed_dir(agent);
|
let proposed_dir = crate::coordinator::Coordinator::agent_proposed_dir(agent);
|
||||||
let applied_dir = crate::coordinator::Coordinator::agent_applied_dir(agent);
|
let applied_dir = crate::coordinator::Coordinator::agent_applied_dir(agent);
|
||||||
if !proposed_dir.exists() {
|
if !proposed_dir.exists() {
|
||||||
|
|
@ -518,3 +538,33 @@ pub fn spawn_question_watchdog(coord: &Arc<Coordinator>, 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());
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
|
||||||
|
|
@ -652,9 +652,11 @@ pub enum ManagerRequest {
|
||||||
Update {
|
Update {
|
||||||
name: String,
|
name: String,
|
||||||
},
|
},
|
||||||
/// Submit a config commit for the user to approve. `commit_ref` is opaque
|
/// Submit a config commit for the user to approve. `commit_ref` must
|
||||||
/// to the host (typically a git sha pointing into the agent's config repo).
|
/// be a commit sha (7-40 hex chars, short or full) in the agent's
|
||||||
/// On approval the host applies the change via `nixos-container update`.
|
/// 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 {
|
RequestApplyCommit {
|
||||||
agent: String,
|
agent: String,
|
||||||
commit_ref: String,
|
commit_ref: String,
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue