Skip to content

refactor(operator,contracts): route operator secrets through a product_contracts port (WS3) #7096

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
BenKurrek wants to merge 2 commits into from
Closed

refactor(operator,contracts): route operator secrets through a product_contracts port (WS3) #7096

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
452a2d6
refactor(operator,contracts): route operator secrets through a produc…
BenKurrek Aug 4, 2026
c735e0c
fix(architecture): put the operator secrets boundary entry on the rig…
BenKurrek Aug 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

22 changes: 16 additions & 6 deletions crates/ironclaw_architecture/tests/reborn_dependency_boundaries.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3414,12 +3414,21 @@ fn boundary_rules() -> Vec<BoundaryRule> {
// LLM providers, rings logs, and controls an OS service; none of
// that needs to see a turn.
//
// `ironclaw_secrets` is deliberately **not** here. WS3's row
// ("tighten direct `secrets` consumers: remove the `webui` and
// `operator` edges via `product_contracts` ports") removes it, and
// PROPOSAL §12.1b names it security-sensitive with "port
// replacements land first". Adding it before that port exists
// would either fail today or force a waiver — the row owns it.
// ✎ **`ironclaw_secrets` joined the list with WS3's row** ("tighten
// direct `secrets` consumers: remove the `webui` and `operator`
// edges via `product_contracts` ports"). PROPOSAL §12.1b required
// the port replacement to land first, and it did, in the same
// change: `ironclaw_product_contracts::operator_secrets::
// OperatorSecretValueStore` is what `LlmKeyStore` now holds, and
// `ironclaw_reborn_composition::RuntimeOperatorSecretValueStore`
// implements it over the substrate. This entry is the "add the
// boundary rule" half of the row and is what stops the edge coming
// back through some later convenience.
//
// The rule is normal-deps only, so a *dev*-dependency would still be
// legal here — and there is deliberately none: the substrate-fidelity
// tests moved to the port's implementor rather than keeping a test
// seam open into the substrate from the products tier.
crate_name: "ironclaw_operator",
forbidden: vec![
"ironclaw_extension_host",
Expand All @@ -3433,6 +3442,7 @@ fn boundary_rules() -> Vec<BoundaryRule> {
"ironclaw_reborn_openai_compat",
"ironclaw_runner",
"ironclaw_scripts",
"ironclaw_secrets",
"ironclaw_slack_extension",
"ironclaw_telegram_extension",
"ironclaw_turns",
Expand Down
7 changes: 7 additions & 0 deletions crates/ironclaw_architecture/tests/reborn_operator_port_inversion.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,12 @@ const INVERTED_PORTS: &[(&str, &str)] = &[
("ActiveModelReader", OPERATOR),
("LlmConfigService", OPERATOR),
("OperatorLogsService", OPERATOR),
// WS3's secrets tightening. Implemented by COMPOSITION for the same reason
// `OperatorStatusService` is: assembly is the only layer that may name both
// the products-tier port and the `ironclaw_secrets` substrate behind it,
// which is the whole point of removing the operator's direct edge
// (PROPOSAL §8.2's product row, §12.1b).
("OperatorSecretValueStore", COMPOSITION),
("OperatorServiceLifecycleService", OPERATOR),
("OperatorStatusService", COMPOSITION),
];
Expand All @@ -92,6 +98,7 @@ const INVERTED_PORTS: &[(&str, &str)] = &[
/// alias" becomes permanent.
const INVERTED_PORT_DTOS: &[&str] = &[
"CodexLoginStart",
"OperatorSecretValueStoreError",
"LlmActiveSelection",
"LlmConfigServiceError",
"LlmConfigSnapshot",
Expand Down
20 changes: 16 additions & 4 deletions crates/ironclaw_operator/AGENTS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,22 @@

## Known Debt

- **Direct `ironclaw_secrets` edge.** The key store reaches the secret store
directly. CHECKLIST WS3 removes it behind a `product_contracts` port
("port replacements land first", PROPOSAL §12.1b), so the boundary rule
deliberately does *not* forbid `ironclaw_secrets` yet.
- ~~**Direct `ironclaw_secrets` edge.**~~ **Discharged by CHECKLIST WS3.** The
key store holds
`ironclaw_product_contracts::operator_secrets::OperatorSecretValueStore`;
`ironclaw_reborn_composition::RuntimeOperatorSecretValueStore` implements it
over the substrate, and the boundary rule now forbids `ironclaw_secrets` here
under every normal dependency kind. **What stays in this crate is policy** —
the `llm_provider_<id>_api_key` handle derivation and the fail-closed
behaviour when a store call errors. What left is every substrate concern: the
scope (the port fixes it, so this crate cannot name one), the lease protocol
behind `read`, and the substrate's error detail (only a stable classification
string crosses). Two tests travelled with the behaviour rather than being
faked here — `read_is_repeatable_across_reloads` and the #4673
production-store reproduction now live with the port's implementor in
`ironclaw_reborn_composition`, because a fake asserting its own repeatability
proves nothing. Adding a secret substrate back to this crate is a boundary
failure, not a convenience.
- **`llm_admin/provider_admin.rs` `include_str!`s CLI source** (into
`crates/ironclaw_reborn_cli/src/commands/config/init.rs`). Inventoried by
`reborn_cross_crate_include_scan.rs`, which is still report-only; CHECKLIST
Expand Down
9 changes: 7 additions & 2 deletions crates/ironclaw_operator/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ ironclaw_product_contracts = { path = "../ironclaw_product_contracts", version =
ironclaw_llm = { path = "../ironclaw_llm", default-features = false, features = ["registry-provider-factory"] }
ironclaw_reborn_config = { path = "../ironclaw_reborn_config" }
ironclaw_safety = { path = "../ironclaw_safety" }
ironclaw_secrets = { path = "../ironclaw_secrets" }
nix = { version = "0.31", default-features = false, features = ["process"] }
libc = "0.2"
secrecy = "0.10"
Expand All @@ -46,7 +45,13 @@ uuid = { version = "1", features = ["v4"] }

[dev-dependencies]
ironclaw_filesystem = { path = "../ironclaw_filesystem", features = ["test-support"] }
ironclaw_secrets = { path = "../ironclaw_secrets" }
# The in-memory `OperatorSecretValueStore` fake. WS3 removed this crate's
# `ironclaw_secrets` edge (PROPOSAL §8.2, §12.1b); the substrate-fidelity tests
# moved to the port's implementor in `ironclaw_reborn_composition`, and what
# stays here drives operator policy through the port.
ironclaw_product_contracts = { path = "../ironclaw_product_contracts", features = [
"test-support",
] }
rust_decimal = "1"
tempfile = "3"
tower = { version = "0.5", features = ["util"] }
Expand Down
226 changes: 66 additions & 160 deletions crates/ironclaw_operator/src/llm_admin/llm_config_service.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1330,18 +1330,12 @@ fn map_admin_error(error: crate::RebornProviderAdminError) -> LlmConfigServiceEr
#[cfg(test)]
mod tests {
use super::*;
use ironclaw_filesystem::{
Fault, FaultInjecting, FilesystemOperation, InMemoryBackend, RootFilesystem,
ScopedFilesystem,
};
use ironclaw_host_api::{
ids::{AgentId, ProjectId, TenantId, UserId},
mount::{MountGrant, MountPermissions, MountView},
path::{MountAlias, VirtualPath},
};
use ironclaw_host_api::ids::{AgentId, ProjectId, TenantId, UserId};
use ironclaw_llm::NEARAI_CLOUD_DEFAULT_BASE_URL;
use ironclaw_product_contracts::test_support::fakes::{
FakeOperatorSecretValueStore, OperatorSecretValueOp,
};
use ironclaw_reborn_config::{RebornHome, RebornProfile};
use ironclaw_secrets::{SecretMaterial, SecretStore};

fn boot_for_home(reborn_home: &std::path::Path) -> RebornBootConfig {
let home = RebornHome::resolve_from_env_parts(
Expand All @@ -1354,79 +1348,58 @@ mod tests {
}

fn key_store() -> LlmKeyStore {
LlmKeyStore::new(Arc::new(SecretStore::ephemeral()))
LlmKeyStore::new(Arc::new(FakeOperatorSecretValueStore::new()))
}

fn secret_store_scoped<F>(root: Arc<F>) -> Arc<ScopedFilesystem<F>>
where
F: RootFilesystem,
{
Arc::new(ScopedFilesystem::with_fixed_view(
root,
MountView::new(vec![MountGrant::new(
MountAlias::new("/secrets").expect("valid secrets alias"),
VirtualPath::new("/engine/secrets").expect("valid secrets target"),
MountPermissions::read_write_list_delete(),
)])
.expect("valid secrets mount"),
/// A working store that counts the port calls made against it.
///
/// ✎ WS3: this used to be the real `SecretStore` over a recording
/// [`FaultInjecting`] backend, and the batching assertion counted
/// *filesystem* ops (`Query` for the batched listing, `ReadFile` for a
/// per-handle probe) to infer which port call the service made. With
/// `LlmKeyStore` on the `OperatorSecretValueStore` port, that inference is
/// unnecessary: the distinction the test exists for — one batched listing
/// versus N per-provider probes — is now directly observable *at the port*,
/// so the assertion moved one layer up and stopped depending on how the
/// substrate happens to implement a metadata read.
fn recording_secret_store() -> Arc<FakeOperatorSecretValueStore> {
Arc::new(FakeOperatorSecretValueStore::new())
}

/// A store whose handle enumeration fails.
///
/// ✎ WS3: this used to be the real `SecretStore` over a [`FaultInjecting`]
/// backend. `ironclaw_operator` no longer names a secret substrate
/// (PROPOSAL §8.2, §12.1b), so the substrate-error-mapping half — that a
/// backend fault becomes `SecretStoreError::StoreUnavailable` and then an
/// `OperatorSecretValueStoreError` carrying `"BackendUnavailable"` — is
/// pinned where it now lives, at the port's implementor
/// (`ironclaw_reborn_composition::operator_secret_store`). What this helper
/// drives is the half that is still this crate's: the service fails loud
/// rather than reporting a snapshot it could not verify.
fn metadata_unavailable_secret_store() -> Arc<FakeOperatorSecretValueStore> {
Arc::new(FakeOperatorSecretValueStore::failing_on(
[
OperatorSecretValueOp::Handles,
OperatorSecretValueOp::Contains,
],
"BackendUnavailable",
))
}

/// The real `SecretStore` over a plain recording [`FaultInjecting`]
/// backend, plus the fault handle. Replaces the former whole-trait
/// `CountingMetadataSecretStore` observer fake: the store now runs its
/// genuine `metadata`/`metadata_for_scope` path and tests count the backend
/// ops it produced (`ReadFile` for a per-handle `metadata`, `Query` for the
/// batched `metadata_for_scope`) instead of a bespoke `AtomicUsize` inside a
/// fake.
fn recording_secret_store() -> (
Arc<SecretStore<FaultInjecting<InMemoryBackend>>>,
Arc<FaultInjecting<InMemoryBackend>>,
) {
let backend = Arc::new(FaultInjecting::new(InMemoryBackend::new()));
let store = Arc::new(SecretStore::ephemeral_over(backend.clone()));
(store, backend)
}

/// The real store over a [`FaultInjecting`] backend armed to fail the secret
/// metadata read paths. Replaces the former `MetadataUnavailableSecretStore`
/// fake: `metadata_for_scope` maps its backing `Query`, and `metadata` its
/// `ReadFile`, so injecting `FaultKind::Backend` on both makes the store's
/// real `FilesystemError::Backend -> SecretStoreError::StoreUnavailable`
/// mapping fire (a `NotFound` fault would instead surface as `Ok(empty)` /
/// `Ok(None)` and never error).
fn metadata_unavailable_secret_store() -> Arc<SecretStore<FaultInjecting<InMemoryBackend>>> {
let backend = Arc::new(
FaultInjecting::new(InMemoryBackend::new())
.with_fault(
Fault::on(FilesystemOperation::Query)
.path("secrets")
.backend("metadata index unavailable"),
)
.with_fault(
Fault::on(FilesystemOperation::ReadFile)
.path("secrets")
.backend("metadata index unavailable"),
),
);
Arc::new(SecretStore::ephemeral_over(backend))
}

/// The real store over a [`FaultInjecting`] backend armed to fail every
/// secret `delete`. Replaces the former `DeleteUnavailableSecretStore` fake
/// (prove provider deletion fails closed when the stored key cannot be
/// removed): the injected backend fault flows through the store's real
/// `delete` -> `FilesystemError::Backend -> SecretStoreError::StoreUnavailable`
/// mapping.
fn delete_unavailable_secret_store() -> Arc<SecretStore<FaultInjecting<InMemoryBackend>>> {
let backend = Arc::new(
FaultInjecting::new(InMemoryBackend::new()).with_fault(
Fault::on(FilesystemOperation::Delete)
.path("secrets")
.backend("secret delete unavailable"),
),
);
Arc::new(SecretStore::ephemeral_over(backend))
/// A store whose `delete` fails while every other operation works.
///
/// Proves provider deletion fails closed when the stored key cannot be
/// removed. Per-operation on purpose: the test has to `put` the key and
/// `contains`-check it afterwards, so an all-failing store could not
/// express the case. ✎ WS3: same note as `metadata_unavailable_secret_store`
/// — the substrate's own `FilesystemError::Backend` mapping is pinned at the
/// port implementor now.
fn delete_unavailable_secret_store() -> Arc<FakeOperatorSecretValueStore> {
Arc::new(FakeOperatorSecretValueStore::failing_on(
[OperatorSecretValueOp::Delete],
"BackendUnavailable",
))
}

fn caller() -> ProductSurfaceCaller {
Expand Down Expand Up @@ -2010,24 +1983,23 @@ mod tests {
let temp = tempfile::tempdir().expect("tempdir");
let reborn_home = temp.path().join("reborn-home");
let boot = boot_for_home(&reborn_home);
let (store, backend) = recording_secret_store();
let service = RebornLlmConfigService::new(boot, LlmKeyStore::new(store));
let store = recording_secret_store();
let service =
RebornLlmConfigService::new(boot, LlmKeyStore::new(Arc::clone(&store) as Arc<_>));

let snapshot = service.snapshot(caller()).await.expect("snapshot");

assert!(
!snapshot.providers.is_empty(),
"snapshot should include registry providers"
);
// `metadata_for_scope` (the batched stored-key listing) is backed by one
// `Query` op; a per-handle `metadata` probe would be a `ReadFile`.
assert_eq!(
backend.count(FilesystemOperation::Query),
store.call_count(OperatorSecretValueOp::Handles),
1,
"snapshot must list stored key metadata once"
"snapshot must list stored key handles once"
);
assert_eq!(
backend.count(FilesystemOperation::ReadFile),
store.call_count(OperatorSecretValueOp::Contains),
0,
"snapshot must not probe stored keys one provider at a time"
);
Expand Down Expand Up @@ -2212,7 +2184,7 @@ mod tests {
let reborn_home = temp.path().join("reborn-home");
let boot = boot_for_home(&reborn_home);
let keys = key_store();
keys.put("nearai", SecretMaterial::from("sk-nearai-test"))
keys.put("nearai", SecretString::from("sk-nearai-test"))
.await
.expect("store nearai key");
let service = RebornLlmConfigService::new(boot, keys);
Expand Down Expand Up @@ -2247,81 +2219,15 @@ mod tests {
);
}

/// Reproduction for issue #4673: saving the NEAR AI (builtin) provider
/// returns `service_unavailable` even though Test connection succeeds. This
/// wires the secret store EXACTLY as production `ironclaw-reborn serve` does
/// — the dynamic `invocation_mount_view` scoped filesystem behind a real
/// `SecretStore` — instead of the in-memory store the other tests
/// use, so a system-scope write/read regression in that path is caught.
#[tokio::test]
async fn upsert_builtin_nearai_with_production_secret_store_succeeds() {
use ironclaw_secrets::{SecretStore, SecretsCrypto};

let temp = tempfile::tempdir().expect("tempdir");
let reborn_home = temp.path().join("reborn-home");
let boot = boot_for_home(&reborn_home);

let backend = Arc::new(ironclaw_filesystem::InMemoryBackend::default());
let scoped = secret_store_scoped(backend);
let crypto = Arc::new(
SecretsCrypto::new(SecretMaterial::from(
"0123456789abcdef0123456789abcdef".to_string(),
))
.expect("valid master key"),
);
let keys = LlmKeyStore::new(Arc::new(SecretStore::new(scoped, crypto)));

let nearai_request = || UpsertLlmProviderRequest {
id: "nearai".to_string(),
client_action_id: None,
name: Some("NEAR AI".to_string()),
adapter: "near_ai".to_string(),
base_url: Some("https://cloud-api.near.ai".to_string()),
default_model: Some("deepseek-ai/DeepSeek-V4-Flash".to_string()),
api_key: Some(SecretString::from("sk-near-test")),
set_active: true,
model: Some("deepseek-ai/DeepSeek-V4-Flash".to_string()),
};

let service = RebornLlmConfigService::new(boot.clone(), keys.clone());
// First save persists the operator's NEAR AI key under the system scope.
let snapshot = service
.upsert_provider(caller(), nearai_request())
.await
.expect("saving the builtin NEAR AI provider must succeed");
let active = snapshot.active.expect("an active provider after save");
assert_eq!(active.provider_id, "nearai");
assert_eq!(
active.model.as_deref(),
Some("deepseek-ai/DeepSeek-V4-Flash")
);

// The stored system-scoped key must read back (the #4673 regression: the
// reserved system tenant id failed to deserialize, so any read-back of a
// system-scoped secret errored — including a second save, which reads the
// previous key first).
assert_eq!(
keys.read("nearai")
.await
.expect("system-scope key must read back")
.expect("a stored key")
.expose_secret(),
"sk-near-test"
);
service
.upsert_provider(caller(), nearai_request())
.await
.expect("re-saving an already-configured NEAR AI provider must succeed");
}
// ✎ WS3: `upsert_builtin_nearai_with_production_secret_store_succeeds` (the
// #4673 system-scope write/read regression) moved to
// `crates/ironclaw_reborn_composition/tests/operator_llm_key_store_wiring.rs`.
// Its whole value is wiring the secret store *exactly as production serve
// does*, and after this crate lost its `ironclaw_secrets` edge (PROPOSAL
// §8.2, §12.1b) "exactly as production" means the real store behind the
// `OperatorSecretValueStore` adapter — a combination only the assembly layer
// can build. It travelled whole; nothing about it was weakened.

/// Integration coverage for the resolver path at the composition boundary
/// (review on #4673): an explicit `config.toml` selection is honored
/// end-to-end through the real `resolve_reborn_runtime_llm`. The env-vs-
/// selection PRECEDENCE itself is unit-tested in
/// `ironclaw_llm::resolution` (`explicit_selection_overrides_env_for_model_and_base_url`),
/// where the env can be set — this crate is `#![forbid(unsafe_code)]` and the
/// resolver reads raw `std::env::var`, so the env dimension cannot be driven
/// here; this thin wrapper only adds the config.toml read it is exercised on.
#[tokio::test]
async fn reborn_runtime_llm_honors_explicit_config_selection() {
let temp = tempfile::tempdir().expect("tempdir");
Expand Down
Loading
Loading