Unify Runtime.secrets and SecretRegistry into one struct
SecretRegistry now holds both declared secrets (SecretString) and
their revealed values (opaque, zeroed on drop). Runtime has a single
registry field instead of separate secrets + registry. Resolving a
secret via (secret :name) reveals and caches the value for redaction
in one step — no more dual bookkeeping.

Assisted-by: GLM-5.1 via pi
change mztxrwvxtnwksvysklyrnyvoqxqlptvr
commit ff08d42a5665e0bfb0d01dd9d2014375033562b3
author Alpha Chen <alpha@kejadlen.dev>
date
parent pspzzvvt
diff --git a/src/ci/redact.rs b/src/ci/redact.rs
index ade00ff..c4462b5 100644
--- a/src/ci/redact.rs
+++ b/src/ci/redact.rs
@@ -1,21 +1,24 @@
 //! Secret redaction for CI output surfaces.
 //!
-//! Collects resolved secret values into a per-run registry and
-//! provides a [`redact`] function that replaces any registered value
-//! with `{{ name }}`.
+//! [`SecretRegistry`] holds both declared secrets (for lookup by name)
+//! and their revealed values (for output redaction). A single struct
+//! replaces the previous split between `Runtime.secrets` and a
+//! separate redaction registry.
 //!
-//! Values are stored opaquely (no [`Debug`] impl, manual [`Drop`]
-//! that overwrites bytes) to avoid re-introducing the secret in
-//! debug output or core dumps. This mirrors the protections in
+//! Revealed values are stored opaquely (no [`Debug`] impl, manual
+//! [`Drop`] that overwrites bytes) to avoid re-introducing the secret
+//! in debug output or core dumps. This mirrors the protections in
 //! `crate::secret::SecretString`.
 
 use std::collections::HashMap;
 
-/// Opaque wrapper for a secret value stored in the registry.
+use crate::secret::SecretString;
+
+/// Opaque wrapper for a revealed secret value.
 /// Zeroes its heap buffer on drop. No Debug impl.
-struct Secret(Vec<u8>);
+struct Revealed(Vec<u8>);
 
-impl Secret {
+impl Revealed {
     fn new(value: String) -> Self {
         Self(value.into_bytes())
     }
@@ -26,7 +29,7 @@ impl Secret {
     }
 }
 
-impl Drop for Secret {
+impl Drop for Revealed {
     fn drop(&mut self) {
         for byte in self.0.iter_mut() {
             *byte = 0;
@@ -34,52 +37,60 @@ impl Drop for Secret {
     }
 }
 
-// Explicitly no Debug impl — the registry must never print secret values.
+// Explicitly no Debug impl — revealed values must never be printed.
 
-/// Per-run collection of secret names and their resolved values.
+/// Per-run secret store: holds declared secrets and their revealed
+/// values for both lookup and redaction.
 ///
-/// Populated as `(secret :name)` is called during CI execution.
-/// Used by [`redact`] to scrub output before persistence.
+/// Constructed with the declared secrets from global config.
+/// As `(secret :name)` is called during CI execution, values are
+/// revealed and cached for redaction via [`redact`].
 ///
 /// Lifetime is bounded to a single CI run. Do not carry a registry
 /// across runs — values from previous runs would contaminate
 /// redaction of unrelated output.
 pub struct SecretRegistry {
-    /// name → value (opaque, zeroed on drop)
-    secrets: HashMap<String, Secret>,
-}
-
-impl Default for SecretRegistry {
-    fn default() -> Self {
-        Self::new()
-    }
+    /// name → declared secret (lazy reveal).
+    declared: HashMap<String, SecretString>,
+    /// name → revealed value (opaque, zeroed on drop).
+    /// Populated on first `(secret :name)` call.
+    revealed: HashMap<String, Revealed>,
 }
 
 impl SecretRegistry {
-    pub fn new() -> Self {
+    pub fn new(declared: HashMap<String, SecretString>) -> Self {
         Self {
-            secrets: HashMap::new(),
+            declared,
+            revealed: HashMap::new(),
         }
     }
 
-    /// Register a resolved secret value under the given name.
-    /// Values shorter than 8 characters are ignored — they're too
-    /// short to redact safely (high false-positive rate on common
-    /// short strings like "set", "yes", "true", "no").
-    pub fn register(&mut self, name: impl Into<String>, value: impl AsRef<str>) {
-        let name = name.into();
-        let value = value.as_ref().to_string();
+    /// Resolve a declared secret by name, caching the revealed value
+    /// for redaction. Returns `Err` if the name isn't declared or
+    /// the source can't be read.
+    ///
+    /// Values shorter than 8 characters are cached for lookup but
+    /// not registered for redaction (high false-positive rate on
+    /// common short strings like "set", "yes", "true", "no").
+    pub fn resolve(&mut self, name: &str) -> super::error::Result<String> {
+        let secret = self
+            .declared
+            .get(name)
+            .ok_or_else(|| super::error::Error::UnknownSecret(name.to_string()))?;
+        let value = secret.reveal()?.to_string();
         if value.len() >= 8 {
-            self.secrets.insert(name, Secret::new(value));
+            self.revealed
+                .insert(name.to_string(), Revealed::new(value.clone()));
         }
+        Ok(value)
     }
 
-    /// Return registered (name, value) pairs sorted by value length
+    /// Return revealed (name, value) pairs sorted by value length
     /// descending so longest matches are replaced first (prevents
     /// partial replacement of overlapping secrets).
     fn entries(&self) -> Vec<(&str, &str)> {
         let mut entries: Vec<_> = self
-            .secrets
+            .revealed
             .iter()
             .map(|(k, v)| (k.as_str(), v.as_str()))
             .collect();
@@ -87,17 +98,17 @@ impl SecretRegistry {
         entries
     }
 
-    pub fn is_empty(&self) -> bool {
-        self.secrets.is_empty()
+    pub fn has_redactions(&self) -> bool {
+        !self.revealed.is_empty()
     }
 }
 
-/// Replace any registered secret value in `text` with `{{ name }}`.
+/// Replace any revealed secret value in `text` with `{{ name }}`.
 ///
 /// Longest values are replaced first to prevent partial matches.
-/// Returns the input unchanged when the registry is empty.
+/// Returns the input unchanged when no secrets have been revealed.
 pub fn redact(text: &str, registry: &SecretRegistry) -> String {
-    if registry.is_empty() {
+    if !registry.has_redactions() {
         return text.to_string();
     }
     let mut result = text.to_string();
@@ -112,10 +123,17 @@ pub fn redact(text: &str, registry: &SecretRegistry) -> String {
 mod tests {
     use super::*;
 
+    fn plain_secrets(pairs: &[(&str, &str)]) -> HashMap<String, SecretString> {
+        pairs
+            .iter()
+            .map(|(k, v)| (k.to_string(), SecretString::from_plain(*v)))
+            .collect()
+    }
+
     #[test]
     fn redact_replaces_secret_value() {
-        let mut reg = SecretRegistry::new();
-        reg.register("github_token", "ghp_abc123xyz");
+        let mut reg = SecretRegistry::new(plain_secrets(&[("github_token", "ghp_abc123xyz")]));
+        reg.resolve("github_token").unwrap();
         assert_eq!(
             redact("push with token ghp_abc123xyz failed", &reg),
             "push with token {{ github_token }} failed"
@@ -124,38 +142,44 @@ mod tests {
 
     #[test]
     fn redact_handles_multiple_secrets() {
-        let mut reg = SecretRegistry::new();
-        reg.register("token_a", "aaaaaaaa");
-        reg.register("token_b", "bbbbbbbb");
+        let mut reg = SecretRegistry::new(plain_secrets(&[
+            ("token_a", "aaaaaaaa"),
+            ("token_b", "bbbbbbbb"),
+        ]));
+        reg.resolve("token_a").unwrap();
+        reg.resolve("token_b").unwrap();
         let result = redact("aaaaaaaa and bbbbbbbb", &reg);
         assert_eq!(result, "{{ token_a }} and {{ token_b }}");
     }
 
     #[test]
     fn redact_longest_first_prevents_partial_overlap() {
-        let mut reg = SecretRegistry::new();
-        reg.register("short", "abcdefgh");
-        reg.register("long", "abcdefghijklmnop");
+        let mut reg = SecretRegistry::new(plain_secrets(&[
+            ("short", "abcdefgh"),
+            ("long", "abcdefghijklmnop"),
+        ]));
+        reg.resolve("short").unwrap();
+        reg.resolve("long").unwrap();
         assert_eq!(redact("abcdefghijklmnop here", &reg), "{{ long }} here");
     }
 
     #[test]
-    fn redact_returns_unchanged_when_empty() {
-        let reg = SecretRegistry::new();
+    fn redact_returns_unchanged_when_nothing_revealed() {
+        let reg = SecretRegistry::new(plain_secrets(&[("key", "secret_value")]));
         assert_eq!(redact("nothing to see", &reg), "nothing to see");
     }
 
     #[test]
     fn redact_ignores_short_secrets() {
-        let mut reg = SecretRegistry::new();
-        reg.register("tiny", "abcdefg");
+        let mut reg = SecretRegistry::new(plain_secrets(&[("tiny", "abcdefg")]));
+        reg.resolve("tiny").unwrap();
         assert_eq!(redact("abcdefg is short", &reg), "abcdefg is short");
     }
 
     #[test]
     fn redact_similar_but_not_equal_passes_through() {
-        let mut reg = SecretRegistry::new();
-        reg.register("token", "ghp_abc123xyz");
+        let mut reg = SecretRegistry::new(plain_secrets(&[("token", "ghp_abc123xyz")]));
+        reg.resolve("token").unwrap();
         assert_eq!(
             redact("ghp_abc124xyz is close but not equal", &reg),
             "ghp_abc124xyz is close but not equal"
@@ -164,8 +188,8 @@ mod tests {
 
     #[test]
     fn redact_replaces_all_occurrences() {
-        let mut reg = SecretRegistry::new();
-        reg.register("key", "secret_password");
+        let mut reg = SecretRegistry::new(plain_secrets(&[("key", "secret_password")]));
+        reg.resolve("key").unwrap();
         assert_eq!(
             redact("secret_password secret_password secret_password", &reg),
             "{{ key }} {{ key }} {{ key }}"
@@ -174,13 +198,30 @@ mod tests {
 
     #[test]
     fn minimum_length_is_8() {
-        let mut reg = SecretRegistry::new();
-        // 7 chars — too short
-        reg.register("short", "1234567");
+        let mut reg =
+            SecretRegistry::new(plain_secrets(&[("short", "1234567"), ("ok", "12345678")]));
+        // 7 chars — too short for redaction
+        reg.resolve("short").unwrap();
         assert_eq!(redact("1234567", &reg), "1234567");
 
         // 8 chars — just enough
-        reg.register("ok", "12345678");
+        reg.resolve("ok").unwrap();
         assert_eq!(redact("12345678", &reg), "{{ ok }}");
     }
+
+    #[test]
+    fn resolve_errors_for_unknown_name() {
+        let mut reg = SecretRegistry::new(plain_secrets(&[]));
+        let err = reg.resolve("missing").unwrap_err();
+        assert!(
+            matches!(err, super::super::error::Error::UnknownSecret(ref n) if n == "missing"),
+            "expected UnknownSecret, got: {err:?}"
+        );
+    }
+
+    #[test]
+    fn resolve_returns_value() {
+        let mut reg = SecretRegistry::new(plain_secrets(&[("key", "hunter2")]));
+        assert_eq!(reg.resolve("key").unwrap(), "hunter2");
+    }
 }
diff --git a/src/ci/runtime.rs b/src/ci/runtime.rs
index b3a86de..551d0e7 100644
--- a/src/ci/runtime.rs
+++ b/src/ci/runtime.rs
@@ -18,7 +18,6 @@ use super::run::{DockerLifecycle, RunMeta};
 use crate::secret::SecretString;
 
 use super::redact::{SecretRegistry, redact};
-
 /// Per-sh timing: (index, started_at, finished_at).
 pub(super) type ShTimings = Vec<(usize, Timestamp, Timestamp)>;
 
@@ -55,7 +54,10 @@ pub(super) enum ExecutorRuntime {
 /// from a VM without one error.
 pub(super) struct Runtime {
     pipeline: Pipeline,
-    pub(super) secrets: HashMap<String, SecretString>,
+    /// Unified secret store: holds declared secrets and their revealed
+    /// values for both lookup and redaction. No Debug impl on the
+    /// registry; Runtime must not derive Debug either.
+    pub(super) registry: RefCell<SecretRegistry>,
     pub(super) inputs: HashMap<String, HashMap<String, Option<mlua::Value>>>,
     pub(super) current_job: RefCell<Option<String>>,
     pub(super) outputs: RefCell<HashMap<String, Vec<ShOutput>>>,
@@ -64,8 +66,6 @@ pub(super) struct Runtime {
     pub(super) sh_timings: RefCell<HashMap<String, ShTimings>>,
     /// Per-job sh call counter for assigning sequential indices.
     sh_counter: RefCell<HashMap<String, usize>>,
-    /// Per-run secret registry for output redaction.
-    registry: RefCell<SecretRegistry>,
     /// The materialized workspace for this run. Every `(sh …)` call
     /// runs here.
     workspace: std::path::PathBuf,
@@ -127,13 +127,12 @@ impl Runtime {
 
         Self {
             pipeline,
-            secrets,
             inputs,
+            registry: RefCell::new(SecretRegistry::new(secrets)),
             current_job: RefCell::new(None),
             outputs: RefCell::new(HashMap::new()),
             sh_timings: RefCell::new(HashMap::new()),
             sh_counter: RefCell::new(HashMap::new()),
-            registry: RefCell::new(SecretRegistry::new()),
             workspace,
             executor,
         }
@@ -199,6 +198,13 @@ impl Runtime {
         std::mem::take(&mut *self.sh_timings.borrow_mut())
     }
 
+    /// Resolve a declared secret by name, caching it for redaction.
+    /// Errors if the name isn't declared or the secret's source
+    /// can't be read.
+    pub(super) fn secret(&self, name: &str) -> super::error::Result<String> {
+        self.registry.borrow_mut().resolve(name)
+    }
+
     /// Borrow the secret registry for redaction.
     ///
     /// Used by run.rs to audit DB columns; kept even though
@@ -208,16 +214,6 @@ impl Runtime {
         self.registry.borrow()
     }
 
-    /// Resolve a declared secret by name. Errors if the name isn't
-    /// declared or the secret's source can't be read.
-    pub(super) fn secret(&self, name: &str) -> super::error::Result<String> {
-        let secret = self
-            .secrets
-            .get(name)
-            .ok_or_else(|| super::error::Error::UnknownSecret(name.to_string()))?;
-        Ok(secret.reveal()?.to_string())
-    }
-
     /// Run `cmd` with `opts` and record its output against the
     /// current job (if one is active). Non-zero exits come back in
     /// `:exit`, not as `Err`.
@@ -293,13 +289,12 @@ impl Runtime {
     fn for_test(pipeline: Pipeline, secrets: HashMap<String, SecretString>) -> Self {
         Self {
             pipeline,
-            secrets,
+            registry: RefCell::new(SecretRegistry::new(secrets)),
             inputs: HashMap::new(),
             current_job: RefCell::new(None),
             outputs: RefCell::new(HashMap::new()),
             sh_timings: RefCell::new(HashMap::new()),
             sh_counter: RefCell::new(HashMap::new()),
-            registry: RefCell::new(SecretRegistry::new()),
             workspace: std::env::current_dir().expect("cwd"),
             executor: ExecutorRuntime::Host,
         }
@@ -347,9 +342,7 @@ impl IntoLua for RuntimeHandle {
             "secret",
             lua.create_function(|lua, name: String| {
                 let rt = runtime(lua)?;
-                let value = rt.secret(&name).map_err(mlua::Error::external)?;
-                rt.registry.borrow_mut().register(&name, &value);
-                Ok(value)
+                rt.secret(&name).map_err(mlua::Error::external)
             })?,
         )?;