[codex] Integration branch for secure/fnox validation work by bglusman · Pull Request #38 · bglusman/calciforge

bglusman · 2026-04-25T03:30:38Z

Summary

Integration-test branch collecting the code-bearing Codex work so reviewers/CI can exercise the stack together without touching other agents' branches.

Includes:

!secure set/list/help chat commands plus FnoxClient stdin-based set wrapper
host-agent validator property coverage, now using valid-input generators rather than filter-heavy arbitrary ASCII
hardened follow-ups from review: real WhatsApp HMAC verification, race-free digest test paths, loopback OneCLI default bind, redacted proxy header logging, bounded fnox subprocess waits
Matrix and WhatsApp !secure wiring so secret commands are intercepted before agent dispatch

Verification

cargo fmt --all --check
cargo test -p adversary-detector digest::tests -- --nocapture
cargo test -p zeroclawed channels::whatsapp::tests::test_verify_hmac_sha256 -- --nocapture
cargo test -p onecli-client
cargo test -p zeroclawed commands::tests::secure_ -- --nocapture
cargo check -p zeroclawed
cargo test -p host-agent accepted_ -- --nocapture
cargo test --workspace --exclude loom-tests
LOOM_MAX_PREEMPTIONS=2 RUSTFLAGS='--cfg loom' cargo test -p loom-tests
pre-push hook passed while pushing branch

Plain cargo test --workspace still trips the known loom guard unless run with RUSTFLAGS='--cfg loom'; this is unchanged from the handoff behavior.

Notes

This is an integration/evaluation target, not a merge request for bypassing the separate PR queue. It intentionally does not push to or mutate other agents' branches.

Adds `!secure` as a post-auth chat command dispatched before any agent sees the message. Subcommands: - `!secure set NAME=value` — stores the secret in fnox and replies with "Stored `NAME`" + a retention-warning. Never echoes the value. - `!secure list` — lists stored secret names (not values). Parses fnox's output defensively; extra columns that look like values are stripped before replying. - `!secure help` / `!secure` — usage string. Dispatch model (matches existing `!switch`/`!sessions`/`!default`): - `CommandHandler::is_secure_command(&str)` — static, case-insensitive. - `CommandHandler::handle_secure(text, identity_id) -> String` — async (shells out to fnox), takes identity for future per-identity audit/ACL though not used yet. Wired into telegram.rs in both `handle_message_nonblocking` and `handle_message` paths, and added to the unknown-command exclusion list so `!secure` doesn't get intercepted as unknown before hitting the post-auth handler. Subprocess approach today — `Command::new("fnox").args(["set", …])`. Acknowledged-cost: three places now shell out to fnox (vault.rs, secure_set, secure_list), which means three places that need an availability check. **Follow-up PR (in flight) will migrate all three to the fnox library crate via `fnox = { git = "https://github.com/jdx/fnox" }`** — fnox already exposes public modules (secret_resolver, commands, etc.) at `src/lib.rs`, so no upstream PR is needed. Redaction discipline: - The success reply names the stored secret but never echoes the value; tested. - The failure reply surfaces fnox's stderr (user needs to know why it failed) but doesn't carry the value; tested. - The telegram channel's `debug!` logs only that a !secure command was handled, without the message text. A `!secure set FOO=bar` would otherwise leak the value into ops logs. - Name validation: `[A-Za-z0-9_-]+` only (matches the substitution engine's accepted ref-name syntax — so a stored secret can be immediately referenced as `{{secret:NAME}}` without a rename). Tested with three invalid-char cases. 5 given/when/then behavioral tests: - `secure_set_reply_includes_name_but_not_value` - `secure_set_surfaces_fnox_error_without_echoing_value` - `secure_unknown_subcommand_returns_help` - `secure_set_rejects_invalid_name_chars` - `secure_list_returns_names_only` All use a fake-fnox shell script installed on PATH via TempDir, so the tests are hermetic regardless of whether the real fnox is installed on the dev/CI machine. Explicit non-goals for this PR (follow-ups): - `!secure remove NAME` — pending a decision on whether fnox's own remove command or a config-file edit is the right approach. - `!secure request NAME` — the out-of-band localhost-paste flow that avoids chat-transport retention entirely (see docs/rfcs/agent-secret-gateway.md §5). - Matrix/WhatsApp channel wiring — same pattern as telegram, but adds separate verification surface. Doing in a follow-up to keep this PR reviewable. - Library-based fnox integration (swap subprocess → lib calls) — committed as next PR per user direction. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Integrates the secure/fnox validation work across the workspace so CI/review can exercise end-to-end secret handling (!secure), improved validation properties, and hardened operational/security behaviors.

Changes:

Adds !secure chat command handling (set/list/help) and intercepts it at channel layers to avoid routing secrets into agent context.
Introduces a consolidated onecli-client::FnoxClient wrapper for invoking fnox (stdin-based set, timeouts, typed errors) and updates related wiring.
Expands host-agent validator coverage (incl. property-based tests) and tightens/limits logging & default binds (e.g., loopback OneCLI bind, redacted proxy header logging, real WhatsApp HMAC verification).

Reviewed changes

Copilot reviewed 16 out of 17 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
crates/zeroclawed/src/proxy/mod.rs	Redacts proxy backend header logging by logging header count only.
crates/zeroclawed/src/commands.rs	Adds `!secure` parsing/handling and a test harness to ensure no value echoing.
crates/zeroclawed/src/channels/whatsapp.rs	Intercepts `!secure` pre-agent dispatch; replaces placeholder webhook HMAC verification with real HMAC-SHA256 verification and tests.
crates/zeroclawed/src/channels/telegram.rs	Ensures `!secure` is treated as a post-auth local command (excluded from unknown-command routing) and handled safely.
crates/zeroclawed/src/channels/matrix.rs	Intercepts and handles `!secure` before unknown-command routing.
crates/onecli-client/src/main.rs	Changes default bind to loopback (127.0.0.1:8081).
crates/onecli-client/src/config.rs	Aligns service config default bind to loopback (127.0.0.1:8081).
crates/onecli-client/src/lib.rs	Exposes the new `fnox_client` module and re-exports `FnoxClient`/`FnoxError`.
crates/onecli-client/src/fnox_client.rs	Adds new consolidated `fnox` subprocess wrapper with timeouts, stdin-based set, and tests.
crates/onecli-client/Cargo.toml	Adds `tempfile` dev-dependency for new fnox client tests.
crates/host-agent/src/zfs/mod.rs	Caches regexes and strengthens dataset/snapshot validation; adds proptest invariants.
crates/host-agent/src/adapters/systemd.rs	Adds proptest invariants for accepted systemd unit names.
crates/host-agent/src/adapters/pct.rs	Adds proptest invariants for accepted Proxmox VMIDs.
crates/host-agent/src/adapters/git.rs	Tightens branch name validation to match git ref safety constraints; adds proptest invariants.
crates/host-agent/Cargo.toml	Adds workspace proptest dev-dependency for new property tests.
crates/adversary-detector/src/digest.rs	Switches digest test temp path generation to `tempfile`.
Cargo.lock	Records dependency additions/updates (e.g., proptest, tempfile).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-25T03:35:47Z

    fn tmp_path() -> PathBuf {
-        // Use a unique path in the system temp dir
-        let dir = std::env::temp_dir();
-        let name = format!(
-            "digest-test-{}-{}",
-            std::process::id(),
-            std::time::SystemTime::now()
-                .duration_since(std::time::UNIX_EPOCH)
-                .unwrap()
-                .as_nanos()
-        );
-        dir.join(name)
+        let dir = tempfile::tempdir()
+            .expect("create digest test temp dir")
+            .keep();
+        dir.join("digests.json")


tmp_path() uses tempfile::tempdir().keep(), which prevents automatic cleanup and will leave temp directories behind on every test run. Prefer keeping the TempDir alive for the duration of each test (e.g., return a struct/tuple containing TempDir + the path), or use a NamedTempFile/TempDir without keep() and build the file path under it.

Addressed in #38 commit fda96d2: tmp_path now returns a TempDir guard plus path so cleanup happens when each test finishes, while still giving every test an isolated directory.

Copilot · 2026-04-25T03:35:47Z

+//! Three call-sites in this workspace shell out to `fnox`:
+//!
+//! - `vault.rs::get_secret_from_fnox` (read path, hot)


The module-level docs mention a vault.rs::get_secret_from_fnox call-site, but there is no such function in crates/onecli-client/src/vault.rs in this branch. Please update these call-site references so the docs accurately reflect current usage of FnoxClient in the workspace.

Suggested change

//! Three call-sites in this workspace shell out to `fnox`:

//!

//! - `vault.rs::get_secret_from_fnox` (read path, hot)

//! This workspace has three `fnox` subprocess use-cases:

//!

//! - secret reads (the hot path)

Addressed in #38 commit fda96d2: module docs now refer to vault.rs::get_secret rather than the old get_secret_from_fnox helper name.

Copilot · 2026-04-25T03:35:47Z

+            .stderr(Stdio::piped())
+            .kill_on_drop(true)
+            .spawn()
+            .map_err(FnoxError::NotInstalled)?;


FnoxClient::set maps any spawn() error to FnoxError::NotInstalled, but spawn failures can also be PermissionDenied, InvalidInput, etc. Consider mapping only ErrorKind::NotFound to NotInstalled and using FnoxError::Io (or a new variant) for other I/O errors so callers don't show a misleading "install fnox" message.

Suggested change

.map_err(FnoxError::NotInstalled)?;

.map_err(|err| match err.kind() {

std::io::ErrorKind::NotFound => FnoxError::NotInstalled(err),

_ => FnoxError::Io(err),

})?;

Addressed in #38 commit fda96d2: spawn errors now map NotFound to NotInstalled and other spawn failures to Io, with a permission-denied regression test.

Copilot · 2026-04-25T03:35:48Z

+            .map_err(|_| FnoxError::TimedOut {
+                seconds: self.timeout.as_secs(),
+            })?
+            .map_err(FnoxError::NotInstalled)


FnoxClient::run maps any Command::output() error to FnoxError::NotInstalled. Similar to set(), this can misclassify permission/configuration errors as "not installed". Recommend branching on std::io::ErrorKind::NotFound for NotInstalled and otherwise returning FnoxError::Io (or a more specific variant).

Suggested change

.map_err(FnoxError::NotInstalled)

.map_err(|err| match err.kind() {

std::io::ErrorKind::NotFound => FnoxError::NotInstalled(err),

_ => FnoxError::Io(err),

})

Addressed in #38 commit fda96d2: FnoxClient::run now uses the same spawn-error classifier, so non-NotFound output/spawn failures no longer masquerade as NotInstalled.

Copilot · 2026-04-25T03:35:48Z

+    // Only allow names that are immediately usable in a `{{secret:NAME}}`
+    // reference. The accepted syntax here is ASCII alphanumeric plus `_`


This comment references a {{secret:NAME}} substitution syntax, but there are no other references to {{secret: in the zeroclawed codebase in this branch. Either align the comment (and potentially the naming constraints) with the actual secret reference mechanism, or link to the code/docs that implement this syntax.

Suggested change

// Only allow names that are immediately usable in a `{{secret:NAME}}`

// reference. The accepted syntax here is ASCII alphanumeric plus `_`

// Only allow names made of ASCII alphanumeric characters plus `_`

Addressed in #38 commit fda96d2: the comment no longer implies current zeroclawed substitution support; it now describes the narrow fnox/future-reference naming constraint.

Copilot · 2026-04-25T03:35:48Z

+fn secure_help() -> String {
+    [
+        "!secure subcommands:",
+        "  !secure set NAME=value    — store a secret by name (never echoed)",
+        "  !secure set NAME value    — same, for mobile keyboards",
+        "  !secure list              — list stored secret names (not values)",
+        "  !secure help              — show this help",
+        "",


!secure is now implemented and has its own secure_help(), but cmd_help() (the output of !help/!commands) still doesn’t mention !secure. If this command is intended to be discoverable for operators, consider adding a brief !secure line to cmd_help() so users can find it without already knowing it exists.

Addressed in #38 commit fda96d2: cmd_help now lists !secure <set|list|help> as an authenticated command.

bglusman · 2026-04-25T18:38:18Z

Subsumed by #44 (squashed to 9ed51fbc on main). All commits from this branch are present in the squash. Closing as redundant rather than merging again.

bglusman and others added 6 commits April 24, 2026 23:22

fix(secure): consolidate fnox calls and avoid argv leaks

3180f24

test(host-agent): add property coverage for validators

553b824

fix(security): harden integration review findings

64c9cb6

feat(secure): wire chat secret commands to matrix and whatsapp

9101d8d

fix(fnox): bound subprocess waits

2e04253

Copilot AI review requested due to automatic review settings April 25, 2026 03:30

Copilot started reviewing on behalf of bglusman April 25, 2026 03:31 View session

test(host-agent): generate valid validator inputs

622df5a

Copilot AI reviewed Apr 25, 2026

View reviewed changes

fix(integration): address pr38 review comments

fda96d2

This was referenced Apr 25, 2026

[integration] All non-conflicting code changes — evaluation target #42

Closed

[super-integration] mine + best of codex — evaluation target #44

Merged

bglusman closed this Apr 25, 2026

bglusman mentioned this pull request Apr 25, 2026

[codex] Add adversarial code review #30

Closed

-            .map_err(FnoxError::NotInstalled)?;
+            .map_err(|err| match err.kind() {
+                std::io::ErrorKind::NotFound => FnoxError::NotInstalled(err),
+                _ => FnoxError::Io(err),
+            })?;

		// Only allow names that are immediately usable in a `{{secret:NAME}}`
		// reference. The accepted syntax here is ASCII alphanumeric plus `_`

	// Only allow names that are immediately usable in a `{{secret:NAME}}`
	// reference. The accepted syntax here is ASCII alphanumeric plus `_`
	// Only allow names made of ASCII alphanumeric characters plus `_`

Conversation

bglusman commented Apr 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Verification

Notes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

Uh oh!

bglusman Apr 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

Uh oh!

bglusman Apr 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

Uh oh!

bglusman Apr 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

Uh oh!

bglusman Apr 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

Uh oh!

bglusman Apr 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

Uh oh!

bglusman Apr 25, 2026

Choose a reason for hiding this comment

Uh oh!

bglusman commented Apr 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bglusman commented Apr 25, 2026 •

edited

Loading