fix(agent): preserve publication source and target semantics

Author: schickling

src/agent_publish.rs

@@ -21,6 +21,7 @@ use crate::catalog_transaction::sync_dir;
 const SCHEMA: &str = "st2.agent-publish.v2";
 const DIGEST_SCHEMA: &str = "st2.agent-source-digest.v1";
 const BUNDLE_DIGEST_DOMAIN: &[u8] = b"st2.agent-publish-bundle.v1\0";
+const CANONICAL_DECLARATION_MODE: u32 = 0o644;
 
 #[derive(Debug, Clone)]
 pub enum PublishSource {
@@ -80,6 +81,12 @@ struct Candidate {
     input_sha256: String,
 }
 
+#[derive(Debug)]
+struct ExistingSpec {
+    bytes: Vec<u8>,
+    mode: u32,
+}
+
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize)]
 #[serde(rename_all = "lowercase")]
 pub enum SourceKind {
@@ -124,14 +131,6 @@ impl Candidate {
             }
         };
         let spec_path = stage.path().join("agent.kdl");
-        anyhow::ensure!(
-            match &source {
-                PublishSource::Spec(path) =>
-                    path.extension().and_then(|value| value.to_str()) == Some("kdl"),
-                PublishSource::Bundle(_) => true,
-            },
-            "published spec must be canonical KDL"
-        );
         let metadata = fs::symlink_metadata(&spec_path)
             .with_context(|| format!("read candidate spec {}", spec_path.display()))?;
         anyhow::ensure!(
@@ -236,16 +235,22 @@ pub fn publish(request: PublishRequest) -> Result<PublishResult> {
         .join(&candidate.identity);
     let target_spec = target_dir.join("agent.kdl");
     validate_existing_ancestry(&catalog, &target_dir)?;
-    let before = read_regular_optional(&target_spec)?;
-    let same_spec = before.as_deref() == Some(candidate.bytes.as_slice());
-    let before_hash = before.as_deref().map(sha256);
+    let before = read_existing_spec(&target_spec)?;
+    let same_spec = before
+        .as_ref()
+        .is_some_and(|current| current.bytes == candidate.bytes);
+    let before_hash = before.as_ref().map(|current| sha256(&current.bytes));
+    let target_mode = before
+        .as_ref()
+        .map(|current| current.mode)
+        .unwrap_or(CANONICAL_DECLARATION_MODE);
     let after_hash = sha256(&candidate.bytes);
 
     match &request.expectation {
         PublishExpectation::Absent => {
             if let Some(current) = &before {
                 anyhow::ensure!(
-                    current == &candidate.bytes,
+                    current.bytes == candidate.bytes,
                     "publish precondition failed: {} already exists with sha256 {}",
                     target_spec.display(),
                     before_hash.as_deref().unwrap_or("<unreadable>")
@@ -331,6 +336,7 @@ pub fn publish(request: PublishRequest) -> Result<PublishResult> {
                 &target_spec,
                 &candidate.bytes,
                 before.is_some(),
+                target_mode,
             )?;
         }
         CandidateKind::Bundle => {
@@ -448,17 +454,24 @@ fn sha256(bytes: &[u8]) -> String {
     format!("{:x}", Sha256::digest(bytes))
 }
 
-fn read_regular_optional(path: &Path) -> Result<Option<Vec<u8>>> {
-    match fs::symlink_metadata(path) {
-        Ok(metadata) => {
+fn read_existing_spec(path: &Path) -> Result<Option<ExistingSpec>> {
+    match OpenOptions::new()
+        .read(true)
+        .custom_flags(libc::O_CLOEXEC | libc::O_NOFOLLOW | libc::O_NONBLOCK)
+        .open(path)
+    {
+        Ok(mut file) => {
+            let metadata = file.metadata()?;
             anyhow::ensure!(
-                metadata.is_file() && !metadata.file_type().is_symlink(),
+                metadata.is_file(),
                 "publication target is not a regular file: {}",
                 path.display()
             );
-            Ok(Some(
-                fs::read(path).with_context(|| format!("read {}", path.display()))?,
-            ))
+            let mode = metadata.permissions().mode() & 0o7777;
+            let mut bytes = Vec::new();
+            file.read_to_end(&mut bytes)
+                .with_context(|| format!("read {}", path.display()))?;
+            Ok(Some(ExistingSpec { bytes, mode }))
         }
         Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(None),
         Err(error) => Err(error).with_context(|| format!("read {}", path.display())),
@@ -674,6 +687,7 @@ fn atomic_write_spec(
     target: &Path,
     bytes: &[u8],
     replace: bool,
+    mode: u32,
 ) -> Result<()> {
     let parent = target.parent().context("spec target has no parent")?;
     let control = crate::catalog_transaction::retained_dir_path(control_file)?;
@@ -682,6 +696,8 @@ fn atomic_write_spec(
         .tempfile_in(&control)
         .with_context(|| format!("create temporary spec in {}", control.display()))?;
     temp.write_all(bytes)?;
+    temp.as_file()
+        .set_permissions(fs::Permissions::from_mode(mode))?;
     temp.as_file().sync_all()?;
     test_crash_after_temporary_write();
     if replace {

tests/agent_publish.rs

@@ -1,6 +1,7 @@
 use std::fs;
 use std::io::Write as _;
-use std::os::unix::process::ExitStatusExt as _;
+use std::os::unix::fs::{MetadataExt as _, PermissionsExt as _};
+use std::os::unix::process::{CommandExt as _, ExitStatusExt as _};
 use std::path::{Path, PathBuf};
 use std::process::Stdio;
 use std::process::{Command, Output};
@@ -113,6 +114,121 @@ fn spec_create_is_typed_and_idempotent() {
     assert_eq!(second["status"], "unchanged");
 }
 
+#[test]
+fn spec_source_filename_is_not_semantic_after_strict_parsing() {
+    let temp = tempfile::tempdir().unwrap();
+    let catalog = temp.path().join("catalog");
+    fs::create_dir(&catalog).unwrap();
+    let candidate = temp.path().join("agent.kdl.candidate");
+    fs::write(&candidate, valid_spec(false)).unwrap();
+
+    let digest = source_digest("--spec", &candidate);
+    let published = publish(&catalog, &candidate, &["--expect-absent"]);
+    assert!(
+        published.status.success(),
+        "{}",
+        String::from_utf8_lossy(&published.stderr)
+    );
+    assert_eq!(digest, sha256(valid_spec(false).as_bytes()));
+
+    let malformed = temp.path().join("malformed.candidate");
+    fs::write(&malformed, "agent \"worker\" {").unwrap();
+    let rejected = st2()
+        .args(["agent", "digest", "--spec"])
+        .arg(&malformed)
+        .output()
+        .unwrap();
+    assert!(!rejected.status.success());
+    assert!(
+        String::from_utf8_lossy(&rejected.stderr).contains("strict declaration parsing"),
+        "{}",
+        String::from_utf8_lossy(&rejected.stderr)
+    );
+}
+
+#[test]
+fn spec_creation_uses_the_canonical_readable_declaration_mode() {
+    let temp = tempfile::tempdir().unwrap();
+    let catalog = temp.path().join("catalog");
+    fs::create_dir(&catalog).unwrap();
+    let candidate = temp.path().join("candidate.kdl");
+    fs::write(&candidate, valid_spec(false)).unwrap();
+    let input_sha256 = sha256(&fs::read(&candidate).unwrap());
+
+    let mut command = st2();
+    command.args([
+        "agent",
+        "publish",
+        "--catalog",
+        catalog.to_str().unwrap(),
+        "--spec",
+        candidate.to_str().unwrap(),
+        "--input-sha256",
+        &input_sha256,
+        "--expect-absent",
+    ]);
+    unsafe {
+        command.pre_exec(|| {
+            libc::umask(0o077);
+            Ok(())
+        });
+    }
+    let published = command.output().unwrap();
+    assert!(
+        published.status.success(),
+        "{}",
+        String::from_utf8_lossy(&published.stderr)
+    );
+    assert_eq!(
+        fs::metadata(target(&catalog)).unwrap().mode() & 0o7777,
+        0o644
+    );
+}
+
+#[test]
+fn spec_replacement_preserves_the_accepted_target_mode() {
+    let temp = tempfile::tempdir().unwrap();
+    let catalog = temp.path().join("catalog");
+    let agent = catalog.join("agents/host/worker");
+    fs::create_dir_all(&agent).unwrap();
+    let current = valid_spec(false);
+    fs::write(agent.join("agent.kdl"), &current).unwrap();
+    fs::set_permissions(agent.join("agent.kdl"), fs::Permissions::from_mode(0o640)).unwrap();
+    let candidate = temp.path().join("candidate.kdl");
+    fs::write(&candidate, valid_spec(true)).unwrap();
+    let input_sha256 = sha256(&fs::read(&candidate).unwrap());
+
+    let mut command = st2();
+    command.args([
+        "agent",
+        "publish",
+        "--catalog",
+        catalog.to_str().unwrap(),
+        "--spec",
+        candidate.to_str().unwrap(),
+        "--input-sha256",
+        &input_sha256,
+        "--expect-sha256",
+        &sha256(current.as_bytes()),
+    ]);
+    unsafe {
+        command.pre_exec(|| {
+            libc::umask(0o077);
+            Ok(())
+        });
+    }
+    let published = command.output().unwrap();
+    assert!(
+        published.status.success(),
+        "{}",
+        String::from_utf8_lossy(&published.stderr)
+    );
+    assert_eq!(
+        fs::metadata(target(&catalog)).unwrap().mode() & 0o7777,
+        0o640
+    );
+}
+
 #[test]
 fn caller_source_digest_rejects_mutation_and_symlink_swaps_before_publication() {
     let temp = tempfile::tempdir().unwrap();