Skip to main content

max / makenotwork

bento: strict artifact resolution, replacing silent sh() glob select Recipes selected artifacts with sh(host, "ls -t <glob> | head -1") guarded only on an empty string. sh() swallows exit codes by design, so a non-zero ls slipped past and the newest-by-mtime file won -- a stale artifact could ship unnoticed. Add resolve_artifact(host, glob) (exactly one match; zero or many throw) and resolve_artifact_opt(host, glob) (zero-or-one; many throw). Ambiguity is an error rather than a newest-wins guess, since >1 match means the build left stale artifacts behind. Decision logic is factored into the pure, unit-tested resolve_artifact_match; the glob metacharacter guard is now a shared ensure_glob_safe helper (collect uses it too). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-07-23 21:03 UTC
Commit: 24c51b5765243a1bb3eae145214a6b3bdc16add6
Parent: c31fa97
1 file changed, +152 insertions, -21 deletions
M bento/daemon/src/engine.rs +152 -21
@@ -391,6 +391,29 @@ impl RecipeCtx {
391 391 .collect();
392 392 Ok((code, tail))
393 393 }
394 +
395 + /// Resolve `glob` on `host` to the single artifact it names. The `for` loop
396 + /// lists each existing match on its own line (and prints nothing — rather
397 + /// than a literal unexpanded pattern — when the glob matches no file), so
398 + /// the count is unambiguous. `required` controls whether zero matches is an
399 + /// error; more than one always is. See `resolve_artifact_match`.
400 + fn resolve_artifact(
401 + self: &Arc<Self>,
402 + host: &str,
403 + glob: &str,
404 + required: bool,
405 + ) -> Result<String> {
406 + ensure_glob_safe(glob)?;
407 + // `[ -e ]` guards against a non-matching glob surviving as its literal
408 + // self, and lists one path per line for the count.
409 + let cmd = format!("for __f in {glob}; do [ -e \"$__f\" ] && printf '%s\\n' \"$__f\"; done");
410 + let (code, tail) = self.run(host, &cmd)?;
411 + anyhow::ensure!(
412 + code == 0,
413 + "resolving artifact glob `{glob}` on `{host}` exited {code}"
414 + );
415 + resolve_artifact_match(&tail, glob, required)
416 + }
394 417 }
395 418
396 419 // ----- error bridging: anyhow -> Rhai runtime error -----
@@ -550,6 +573,52 @@ pub fn expand_tilde(p: &str) -> PathBuf {
550 573 PathBuf::from(p)
551 574 }
552 575
576 + /// Reject a glob that carries shell command metacharacters. Path and wildcard
577 + /// characters (`/ . * ? [ ] ~` etc.) are fine — the pattern reaches a login
578 + /// shell to be expanded — but a `;` or `$(...)` must not ride along and run.
579 + /// Not a privilege boundary (a recipe already runs arbitrary shell via `sh_ok`)
580 + /// but it keeps a malformed pattern from turning into a command. Shared by
581 + /// `collect` and `resolve_artifact`.
582 + fn ensure_glob_safe(glob: &str) -> Result<()> {
583 + anyhow::ensure!(
584 + !glob.chars().any(|c| matches!(
585 + c,
586 + ';' | '&' | '|' | '$' | '`' | '\'' | '"' | '\\' | ' ' | '\n' | '(' | ')' | '<' | '>'
587 + )),
588 + "glob `{glob}` contains shell metacharacters"
589 + );
590 + Ok(())
591 + }
592 +
593 + /// Decide the single artifact a glob resolves to from a newline-separated
594 + /// listing of the paths that matched it.
595 + ///
596 + /// The recipes used to select an artifact with `ls -t <glob> | head -1` and
597 + /// guard only on an empty string, so a non-zero `ls` slipped past quietly and a
598 + /// stale newest-by-mtime file could win. This is the strict replacement: it
599 + /// demands exactly one match. Zero matches fail when `required` (return `""`
600 + /// when optional); more than one is always an error rather than an arbitrary
601 + /// newest-wins pick, because an ambiguous match means the build left stale
602 + /// artifacts behind and the wrong one could ship.
603 + fn resolve_artifact_match(listing: &str, glob: &str, required: bool) -> Result<String> {
604 + let matches: Vec<&str> = listing
605 + .lines()
606 + .map(str::trim)
607 + .filter(|l| !l.is_empty())
608 + .collect();
609 + match matches.as_slice() {
610 + [] if required => anyhow::bail!("no artifact matched glob `{glob}`"),
611 + [] => Ok(String::new()),
612 + [one] => Ok((*one).to_string()),
613 + many => anyhow::bail!(
614 + "glob `{glob}` is ambiguous: {} artifacts matched ({}). \
615 + The build left more than one behind; clean stale artifacts so exactly one remains.",
616 + many.len(),
617 + many.join(", ")
618 + ),
619 + }
620 + }
621 +
553 622 /// Build a Rhai engine with the host API bound to `ctx`. Sandboxed: recipes
554 623 /// touch the outside world only through these functions.
555 624 pub fn build_engine(ctx: Arc<RecipeCtx>) -> Engine {
@@ -616,6 +685,40 @@ pub fn build_engine(ctx: Arc<RecipeCtx>) -> Engine {
616 685 );
617 686 }
618 687
688 + // --- resolve_artifact(host, glob) -> path: the ONE artifact matching glob ---
689 + //
690 + // The artifact-selection primitive. Replaces `sh(host, "ls -t <glob> | head
691 + // -1").stdout_tail.trim()` guarded on an empty string, which let a non-zero
692 + // `ls` pass quietly and a stale newest-by-mtime file win. This resolves the
693 + // glob on the host and demands exactly one match: zero matches or more than
694 + // one both throw (an ambiguous match means the build left stale artifacts,
695 + // and silently picking the newest is how the wrong bytes ship). Use
696 + // `resolve_artifact_opt` for an artifact that may legitimately be absent.
697 + {
698 + let ctx = ctx.clone();
699 + engine.register_fn(
700 + "resolve_artifact",
701 + move |host: &str, glob: &str| -> Result<String, Box<EvalAltResult>> {
702 + ctx.resolve_artifact(host, glob, true).map_err(rhai_err)
703 + },
704 + );
705 + }
706 +
707 + // --- resolve_artifact_opt(host, glob) -> path | "": zero-or-one match ---
708 + //
709 + // Same strict resolution as `resolve_artifact` but tolerates zero matches
710 + // (returns ""); more than one is still an error. For optional outputs like a
711 + // `.deb` or an updater bundle a recipe collects only when present.
712 + {
713 + let ctx = ctx.clone();
714 + engine.register_fn(
715 + "resolve_artifact_opt",
716 + move |host: &str, glob: &str| -> Result<String, Box<EvalAltResult>> {
717 + ctx.resolve_artifact(host, glob, false).map_err(rhai_err)
718 + },
719 + );
720 + }
721 +
619 722 // --- log(msg): operator-visible line into the current step's tail ---
620 723 {
621 724 let ctx = ctx.clone();
@@ -876,27 +979,7 @@ impl RecipeCtx {
876 979 let dest_s = dest.to_string_lossy().into_owned();
877 980 // The glob reaches a remote login shell intact (that's what expands it),
878 981 // so command metacharacters stay barred. Path/wildcard chars are fine.
879 - // Not a privilege boundary — a recipe already runs arbitrary shell via
880 - // `sh_ok` — but it keeps a malformed pattern from becoming a command.
881 - anyhow::ensure!(
882 - !glob.chars().any(|c| matches!(
883 - c,
884 - ';' | '&'
885 - | '|'
886 - | '$'
887 - | '`'
888 - | '\''
889 - | '"'
890 - | '\\'
891 - | ' '
892 - | '\n'
893 - | '('
894 - | ')'
895 - | '<'
896 - | '>'
897 - )),
898 - "collect glob `{glob}` contains shell metacharacters"
899 - );
982 + ensure_glob_safe(glob)?;
900 983 std::fs::create_dir_all(&dest)
901 984 .with_context(|| format!("creating collect dest {dest_s}"))?;
902 985 // The SYNC transport, not the host's exec executor: artifacts move over
@@ -1451,6 +1534,54 @@ mod tests {
1451 1534 assert_eq!(expand_tilde("/abs/path"), PathBuf::from("/abs/path"));
1452 1535 }
1453 1536
1537 + // ---- artifact resolution (the M3 silent-`sh` fix) ----
1538 +
1539 + #[test]
1540 + fn resolve_artifact_match_wants_exactly_one() {
1541 + // Exactly one match: the path, trimmed of the listing's line noise.
1542 + assert_eq!(
1543 + resolve_artifact_match(" /d/App.AppImage \n", "*.AppImage", true).unwrap(),
1544 + "/d/App.AppImage"
1545 + );
1546 + }
1547 +
1548 + #[test]
1549 + fn resolve_artifact_match_zero_depends_on_required() {
1550 + // Required + zero matches is the case the old empty-string guard caught;
1551 + // keep failing it.
1552 + let err = resolve_artifact_match("", "*.dmg", true).unwrap_err();
1553 + assert!(err.to_string().contains("no artifact matched"), "{err}");
1554 + // Optional + zero matches resolves to empty (recipe skips the collect).
1555 + assert_eq!(
1556 + resolve_artifact_match("\n \n", "*.deb", false).unwrap(),
1557 + ""
1558 + );
1559 + }
1560 +
1561 + #[test]
1562 + fn resolve_artifact_match_rejects_ambiguous() {
1563 + // Two matches must throw rather than silently pick one — this is the
1564 + // stale-newest-mtime hole the audit flagged. Applies even when optional.
1565 + for required in [true, false] {
1566 + let err =
1567 + resolve_artifact_match("/d/old.deb\n/d/new.deb\n", "*.deb", required).unwrap_err();
1568 + let msg = err.to_string();
1569 + assert!(msg.contains("ambiguous"), "{msg}");
1570 + assert!(msg.contains("old.deb") && msg.contains("new.deb"), "{msg}");
1571 + }
1572 + }
1573 +
1574 + #[test]
1575 + fn ensure_glob_safe_allows_paths_bars_commands() {
1576 + // Path and wildcard characters pass.
1577 + assert!(ensure_glob_safe("~/Code/app/dist/*.AppImage").is_ok());
1578 + assert!(ensure_glob_safe("/t/App_1.2.3-x86_64.dmg").is_ok());
1579 + // A command substitution or separator does not.
1580 + for bad in ["*.dmg; rm -rf /", "$(evil)", "a|b", "a b"] {
1581 + assert!(ensure_glob_safe(bad).is_err(), "should reject {bad:?}");
1582 + }
1583 + }
1584 +
1454 1585 // ---- version resolution ----
1455 1586
1456 1587 #[test]