Skip to content

arch(reborn): futureproof RebornRuntime surface for epic #3036 (Configuration-as-Code) - #3703

Closed
serrrfirat wants to merge 4 commits into
mainfrom
reborn/runtime-surface-futureproof-3036
Closed

serrrfirat wants to merge 4 commits into
mainfrom
reborn/runtime-surface-futureproof-3036

Conversation

@serrrfirat

@serrrfirat serrrfirat commented May 16, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Reshapes RebornRuntimeInput / RebornRuntime / the ironclaw-reborn CLI command tree so that epic #3036 — "Configuration-as-Code for IronClaw Reborn: tenant blueprints and use-case harnesses" can land its substrate (typed repos, blueprint parser, apply service, harness registry, instruction-bundle assembler) without breaking the standalone Reborn binary's input contract, runtime handle, or operator-facing CLI surface.

This is a forward-architecture PR: nearly every line is additive seams + DTOs + stubs + boundary tests. Behavior of the running agent is unchanged.

Stacked on top of #3695 (composition root + narrow surface + live binary).

How epic #3036 shows up in this PR

Epic #3036 concept Today's shape (this PR) When the substrate lands
api_version = "ironclaw.config/v1" on every blueprint RebornRuntimeApiVersion::V1 mandatory field on RebornRuntimeInput; major mismatch is RebornRuntimeError::IncompatibleApiVersion (fail-closed) Same enforcement; minor bumps land additive fields
Tenant blueprint [scope] tenant = "acme" + per-tenant identity RebornIdentityConfig { tenant, default_agent, default_owner, default_project } first-class field; ConversationIdentity per-conversation override DTO is populated from the parsed blueprint via the apply service writing through RebornTenantRepo; runtime reads from repo
Blueprint [runtime] profile + approval policy RebornPolicyConfig { deployment_mode, default_profile, default_approval_policy } recorded in boot log via tracing::debug Resolved per-request via RuntimePolicyRepo lookup; default written by blueprint apply
Blueprint [providers.<name>] LLM config RebornLlmConfig already on the input (PR #3695); CLI env-resolves today Sourced from ProviderRepo populated by blueprint apply
HarnessManifest activation RebornHarnessSelection { harness_id } optional input field; logged + no-op today Drives HarnessActivationService → InstructionBundleAssembler overlay + capability-surface filter; per-message override via SendMessageOptions::with_harness
BlueprintApplyService as a privileged CapabilityHost workflow RebornAdminClient handle with apply_blueprint / diff_blueprint returning RebornAdminError::NotYetWired Method bodies replaced; signatures unchanged
HarnessRepo lifecycle RebornAdminClient::{install_harness, activate_harness, deactivate_harness, list_harnesses} returning NotYetWired Method bodies replaced; signatures unchanged
AdminScope privilege gate (epic explicitly says "reuse AdminScope::new from src/tenant.rs") RebornAdminScope sealed via private SealedMarker; for_cli_admin(audit_reason) is the only public constructor Gains for_authenticated_admin(credential, audit_reason) when the privilege/auth crate lands
ironclaw config apply / diff / watch CLI commands Discoverable stub subcommands with final argument shape, fail with structured tracking_issue: #3036 errors Bodies switch from fail_not_yet_wired(...) to runtime.admin(scope).apply_blueprint(...).await
ironclaw harness install / list / activate / deactivate Same — discoverable stubs with final argument shape Same — body swap-in only

Every row above is a future PR that swaps an Err(NotYetWired) for a real body — not a future PR that changes the shape of RebornRuntimeInput, RebornRuntime::admin(scope), RebornAdminClient, or the CLI subcommand tree.

Stack of commits

Commit Purpose
arch(reborn-composition): futureproof RebornRuntimeInput for epic #3036 Splits runtime_input.rs into a module; adds RebornRuntimeApiVersion, RebornIdentityConfig, ConversationIdentity, RebornPolicyConfig, RebornDriverConfig/RebornDriverChoice, RebornHarnessSelection/RebornHarnessId. Compatibility wired in build_reborn_runtime (api_version check, planned-driver = NotYetWired, harness selection = log+proceed). Unit tests per DTO.
arch(reborn-composition): add RebornAdminClient + sealed RebornAdminScope for epic #3036 New runtime_admin.rs. RebornAdminScope sealed via private SealedMarker. RebornAdminClient with 6 typed async methods, each returning RebornAdminError::NotYetWired { operation, tracking_issue: "#3036", requires: "<substrate>" }. RebornRuntime::admin(scope) accessor + RebornRuntime::new_conversation_for(ConversationIdentity) + SendMessageOptions { requested_run_profile, harness_override } + send_user_message_with_options(...). Per-conversation identity now tracked in an Arc<RwLock<HashMap<ThreadId, ConversationContext>>> so multi-tenant new_conversation_for is real, not hardcoded.
feat(reborn-cli): lock epic #3036 operator surface as discoverable stubs Splits commands/config.rs → commands/config/{mod,apply,diff,watch}.rs; new commands/harness/{mod,install,list,activate,deactivate}.rs; shared commands/stubs.rs for uniform "not yet wired" output. 8 new CLI smoke tests pinning the operator-facing stub surface.
arch(boundaries): pin RebornRuntime futureproof surface for epic #3036 New boundary test reborn_runtime_surface_is_futureproof_for_epic_3036 asserts (1) all 5 epic-3036 fields on RebornRuntimeInput, (2) every admin method present, (3) RebornAdminScope remains sealed (private _sealed: SealedMarker field), (4) every CLI stub file exists.

What the runtime looks like to callers now

use ironclaw_reborn_composition::{
    build_reborn_runtime,
    // versioned input
    RebornRuntimeApiVersion, RebornRuntimeInput,
    // independent input layers (forward-compatible with epic #3036)
    RebornIdentityConfig, ConversationIdentity,
    RebornPolicyConfig,
    RebornDriverConfig, RebornDriverChoice,
    RebornHarnessSelection, RebornHarnessId,
    RebornLlmConfig,                   // #[cfg(feature = "root-llm-provider")]
    TurnRunnerSettings,
    // agent surface
    RebornRuntime, ConversationId, AssistantReply, SendMessageOptions,
    // admin surface
    RebornAdminClient, RebornAdminScope, RebornAdminError,
    // admin DTOs reserved for epic #3036
    ApplyBlueprintRequest, ApplyMode, ApplyReport, ApplyChange, ApplyChangeAction,
    DiffBlueprintRequest, BlueprintSource,
    InstallHarnessRequest, ActivateHarnessRequest, DeactivateHarnessRequest,
    HarnessActivationScope, HarnessDescriptor,
};

That is the entire surface a downstream consumer touches. No TurnCoordinator, no SessionThreadService, no HostManagedModelGateway, no SubmitTurnRequest, no host-api substrate types — everything stays inside the composition root.

// Agent surface
let conv = runtime.new_conversation_for(
    ConversationIdentity::for_tenant(acme).with_agent(acme_bot)
).await?;
let reply = runtime.send_user_message_with_options(
    &conv,
    "hello",
    SendMessageOptions::default()
        .with_requested_run_profile("text_only")
        .with_harness(red_team),     // recorded today, takes effect when harness substrate lands
).await?;

// Admin surface
let scope = RebornAdminScope::for_cli_admin("operator ran `config apply`");
let admin = runtime.admin(scope);
let report = admin.apply_blueprint(ApplyBlueprintRequest {
    source: BlueprintSource::Path(blueprint_path),
    mode: ApplyMode::Apply,
    lockfile: Some(lockfile_path),
}).await?;
// Today: returns Err(NotYetWired { operation: "config.apply", tracking_issue: "#3036", requires: "..." })
// Tomorrow (epic sub-issue lands): returns Ok(ApplyReport { changes: [...] })

Operator surface verified

$ ironclaw-reborn --help
...
  harness     Install / activate / list use-case harnesses (epic #3036).
              Subcommands ship as discoverable stubs today; substrate
              lands incrementally via epic sub-issues.
...

$ ironclaw-reborn config --help
...
  path   Show resolved Reborn configuration paths without creating state
  apply  Apply a declarative IronClaw blueprint to the runtime's typed repos
  diff   Diff a declarative blueprint against the runtime's typed repos without writing
  watch  Watch a git URL for blueprint revisions and apply on each new revision (GitOps mode)

$ ironclaw-reborn harness --help
...
  install     Install a harness manifest into the typed harness repo
  list        List installed harnesses
  activate    Activate an installed harness for a scope (thread / project / tenant)
  deactivate  Deactivate the active harness for a scope

$ ironclaw-reborn config apply --dry-run /tmp/x
ironclaw-reborn: command `config.apply` is not yet wired
  tracking issue : #3036
  requires       : BlueprintParser + BlueprintApplyService + typed repos
                   (Settings, Skill, Mission, Project)

The command shape (arguments, output format) is locked from epic #3036;
the substrate that makes it execute lands incrementally via the issue's
sub-issues. Re-run after the relevant sub-issue ships.
$ echo $?
1

Tests

Crate Result Notes
ironclaw_llm 733/733 unchanged
ironclaw_reborn (with root-llm-provider) 225/225 unchanged
ironclaw_reborn_composition (both feature variants) 23/23 +18 unit tests across runtime_input/ submodules + runtime_admin
ironclaw_reborn_cli 43/43 +8 stub-surface smoke tests
ironclaw_architecture 18/18 +1 new boundary test (reborn_runtime_surface_is_futureproof_for_epic_3036)

End-to-end smoke verified: ironclaw-reborn run -m ping without LLM env vars still surfaces RecoveryRequired in ~2s, same as PR #3695. The futureproof reshape did not regress the live-agent path.

What this PR is NOT

How epic #3036 sub-issues consume this PR

Each sub-issue from the epic's "Suggested decomposition" reads as a body-swap on top of this shape:

  1. Blueprint format + parser + JSON Schemas. New ironclaw_blueprint crate. No runtime touch. (Sub-issue 1.)
  2. BlueprintApplyService + per-domain reconcilers + ApplyReport/lockfile. Implements the body of RebornAdminClient::apply_blueprint + ::diff_blueprint. Composition gains an optional Arc<dyn BlueprintApplyService> substrate field. (Sub-issue 2.)
  3. System prompt as scoped setting + InstructionBundleAssembler. New ironclaw_settings crate. Composition starts passing with_identity_context_source(...) to RebornLoopDriverHostFactory. RebornPolicyConfig becomes read-through to RuntimePolicyRepo. (Sub-issue 3.)
  4. HarnessManifest typed repo + lifecycle + activation gating. New ironclaw_harness crate. Implements the body of RebornAdminClient::install_harness / activate_harness / deactivate_harness / list_harnesses. RebornHarnessSelection becomes live: composition wires the active harness's capability-surface filter via with_safety_context(...) and its prompt overlay via the instruction-bundle assembler from sub-issue 3. (Sub-issue 4.)
  5. Reference harness: red-team. Pure content. Lands under harnesses/red-team/. (Sub-issue 5.)
  6. Reference harness: chain-incident-response. Pure content. (Sub-issue 6.)
  7. CLI surface (apply / diff / watch) + admin web endpoint. CLI stub bodies replaced with real calls through runtime.admin(scope).apply_blueprint(...). Admin web is an orthogonal frontend concern. (Sub-issue 7.)
  8. Migration tests + drift detection + audit/event coverage. Tests on top of the wired apply service. (Sub-issue 8.)

At each step, RebornRuntimeInput, RebornRuntime, RebornAdminClient, and the CLI command tree do not change shape — only the bodies inside method definitions, and (orthogonally) the composition's wiring inside build_reborn_runtime. External tooling and operator muscle memory hold steady.

Verification checklist (against epic acceptance criteria)

The epic enumerates many AC bullets; this PR doesn't satisfy them (none of the substrate is built), but several structural prerequisites the AC implicitly depends on are now in place:

  • "Apply must be admin-scoped" — RebornAdminClient is only obtainable via RebornRuntime::admin(scope: RebornAdminScope); scope is sealed and pinned by a boundary test.
  • "API version is mandatory and locks the schema major" — RebornRuntimeApiVersion::V1 mandatory; major mismatch fail-closed; pinned by unit test.
  • "Inline secret material rejected at parse time" — (not in this PR; lands with parser in sub-issue 1)
  • "apply_blueprint(blueprint, scope, mode = Apply | DryRun) returns a typed ApplyReport" — signature exists; body is NotYetWired. Body lands in sub-issue 2.
  • "Change { domain, key, action: Create|Update|NoOp|Delete-deferred, before_hash, after_hash }" — ApplyChange carries exactly those fields.
  • "Active harness reflected in visible_capabilities() before next model call" — (seam exists via RebornLoopDriverHostFactory::with_safety_context; wired by sub-issue 4)
  • "Switching harness emits harness.deactivated then harness.activated audit events" — (audit emission in sub-issue 4)
  • "harness install / list / activate / deactivate exist on the CLI" — stubs ship today with the exact argument shape; sub-issue 4 / sub-issue 7 swap bodies.
  • "ironclaw config apply <path> produces a lockfile" — argument exists; body in sub-issue 2.

Why this PR is small and why it's now

~1k LOC. Every line is one of three things: (a) a typed DTO that didn't exist, (b) a stubbed method body whose signature is committed by the epic, or (c) a boundary test pinning the shape. There is no behavior change to bisect against, no agent-loop touch.

Shipping the reshape now, before any sub-issue starts, means epic #3036's sub-issues can land independently as body-swaps without each one re-litigating "what's the right input shape" or "where does the admin gate live." That conversation is settled here.

The alternative — extending RebornRuntimeInput incrementally as each sub-issue lands — would force five separate breaking changes to the public input contract, force the CLI command tree to grow new top-level subcommands across releases (breaking shell completion expectations), and re-decide the privilege gate on each sub-issue. Concentrating the shape change here is the cheapest version of futureproofing.

cc @ilblackdragon (this addresses the "too many things exposed; just those few structs/traits" feedback that originated this work — every type in the new public surface is one of [DTO, handle, error, ID]; nothing exposes underlying substrate traits).

Splits the previously flat `runtime_input.rs` (api_version-less, with
only `services + llm + runner` fields) into a versioned input contract
that anticipates the layers epic
[#3036](#3036)
("Configuration-as-Code for IronClaw Reborn: tenant blueprints and
use-case harnesses") will write to the runtime.

The shape change is **additive** — every previously-supported invocation
of `RebornRuntimeInput::from_services(svc).with_llm(llm).with_runner_
settings(s)` still compiles unchanged — and **forward-compatible**:
identity, policy, drivers, and harness slots are now first-class fields
with safe CLI-shaped defaults today, so blueprint apply / harness
activation can land in epic sub-issues without breaking the input
contract.

New DTOs (composition-owned, no escaping ironclaw_reborn / ironclaw_llm
types):

  - RebornRuntimeApiVersion (V1) — schema stamp. Composition rejects
    different majors fail-closed. Mirrors the
    `api_version = "ironclaw.config/v1"` discipline epic #3036 puts on
    declarative blueprints.

  - RebornIdentityConfig { tenant, default_agent, default_owner,
    default_project } — replaces the hardcoded "reborn-cli" strings
    that lived inside build_reborn_runtime. `cli_default()` ships the
    safe single-tenant local-dev shape. Once epic #3036's tenant repo
    lands, this DTO will be sourced from RebornTenantRepo instead.

  - ConversationIdentity — per-conversation override consumed by
    `new_conversation_for` (added in a sibling commit). Any field
    `None` falls back to the runtime's default identity.

  - RebornPolicyConfig { deployment_mode, default_profile,
    default_approval_policy } — uses existing
    `ironclaw_host_api::runtime_policy::{DeploymentMode, RuntimeProfile,
    ApprovalPolicy}`. Values are recorded and logged at boot today
    (`build_reborn_runtime` emits a tracing::debug snapshot); per-request
    enforcement lands with RuntimePolicyRepo in epic #3036.

  - RebornDriverConfig { default, additional } over
    RebornDriverChoice::{TextOnly, Planned}. Today's composition wires
    only TextOnly end-to-end; selecting Planned as default returns a
    clear `InvalidArgument` from `build_reborn_runtime` rather than
    silently falling back. The DTO ships now so operator-facing config
    (and the future blueprint TOML) has a typed driver-selection
    surface and the `requested_run_profile` per-turn mechanism (added
    in a sibling commit) has a registry to look up against.

  - RebornHarnessSelection { harness_id: RebornHarnessId } — forward-
    compat slot for the harness substrate (HarnessRepo,
    HarnessActivationService, capability-surface filter, instruction-
    bundle overlay) defined by epic #3036's HarnessManifest. Composition
    today logs the harness id once at boot and proceeds without an
    overlay; the same field starts driving activation when the
    substrate lands. RebornHarnessId carries the same kebab-case rules
    as v1 PackageId so a future migration to a typed
    `ironclaw_harness::HarnessId` is a re-export, not a value
    transformation.

Compatibility wiring inside build_reborn_runtime:

  - api_version is checked against the composition's compiled
    RebornRuntimeApiVersion::current(); mismatch returns
    RebornRuntimeError::IncompatibleApiVersion.
  - drivers.default == Planned returns InvalidArgument today
    (recognized name, not yet wired body).
  - harness selection logs at WARN and proceeds.
  - identity drives the default ThreadScope built inside the runtime,
    replacing the previous hardcoded "reborn-cli" tenant/agent strings.

Tests (unit, in submodules):
  - api_version: V1 is current, major mismatch fails closed, wire
    format "ironclaw.runtime/vM.N" is stable.
  - identity: cli_default() produces the expected stable scope;
    ConversationIdentity builder chain composes.
  - policy: cli_default() returns local-single-user / LocalDev /
    AskDestructive; builders compose.
  - drivers: text_only_only() is the no-extras shape; with_additional
    dedupes against the default and itself.
  - harness: RebornHarnessId validation rejects empty, too long,
    non-lowercase-leading, and invalid characters; serde round-trips.
…cope for epic #3036

Epic [#3036](#3036)
("Configuration-as-Code") introduces operations that are fundamentally
privileged effects: blueprint apply writes to typed settings / skill /
mission / project repos; harness install / activate / deactivate
manipulate the typed harness repo and bump capability-surface version.
The epic explicitly calls out:

> Apply must be admin-scoped for anything beyond `scope = { user =
> self }`. Reuse AdminScope::new from src/tenant.rs; do not invent a
> new privilege gate.

This commit lands the *shape* of that admin gate in the standalone
Reborn composition root so the substrate work can land incrementally
without re-deciding the privilege model in each sub-issue.

  RebornAdminScope (sealed):
    - Carries a human-readable audit_reason recorded against every
      admin write under this scope.
    - Sealed via a private SealedMarker field. The only public
      constructor today is `for_cli_admin(audit_reason)`; remote /
      authenticated admin paths get their own constructor when the
      privilege/auth crate from epic #3036 lands (will add a
      `for_authenticated_admin(credential, audit_reason)` then).
    - Boundary test pins the sealed property
      (reborn_runtime_surface_is_futureproof_for_epic_3036).

  RebornAdminClient:
    - Constructed via `RebornRuntime::admin(scope)` (added in a
      sibling commit). Outside crates cannot construct one directly —
      `pub(crate) fn new(...)` keeps the constructor inside
      ironclaw_reborn_composition.
    - Exposes typed async methods for every privileged op epic #3036
      lists: apply_blueprint, diff_blueprint, install_harness,
      activate_harness, deactivate_harness, list_harnesses.
    - Each method returns RebornAdminError::NotYetWired today with
      precise pointers — operation name, tracking issue "#3036", and
      the specific substrate it needs ("BlueprintParser +
      BlueprintApplyService + typed repos (Settings, Skill, Mission,
      Project)", etc.). Operators hitting the error get a precise
      next-step pointer instead of "feature unimplemented".

  Typed DTOs reserved for the operator surface (today opaque, take
  effect when the substrate lands):
    - ApplyBlueprintRequest / DiffBlueprintRequest with
      BlueprintSource::{Path, GitUrl}, ApplyMode::{DryRun, Apply},
      optional lockfile path.
    - ApplyReport / ApplyChange / ApplyChangeAction matching the
      `Change { domain, key, action, before_hash, after_hash }` shape
      epic #3036 specifies.
    - InstallHarnessRequest, ActivateHarnessRequest,
      DeactivateHarnessRequest, HarnessActivationScope::{Thread,
      Project, Tenant} matching the activation-scope vocabulary from
      the epic.
    - HarnessDescriptor for `list_harnesses` (id, installed_revision,
      active_for_threads).

  RebornAdminError variants:
    - NotYetWired (today's surface; carries operation + tracking
      issue + required substrate).
    - Unauthorized (reserved — surfaces once the privilege/auth
      crate lands and admin scope construction grows a credential
      parameter).
    - Repo (reserved — surfaces once the typed repos exist and
      transactional writes can fail).

  Unit tests:
    - admin_scope_carries_audit_reason: round-trips the audit string.
    - every_admin_method_returns_not_yet_wired: ensures every
      published method surfaces the documented stub error so a
      regression that quietly turns one into a panic / silent
      success fails fast.
Epic [#3036](#3036) lists the
operator CLI surface its substrate eventually drives:

  ironclaw config apply <path|git-url> [--dry-run] [--lockfile]
  ironclaw config diff <path|git-url>
  ironclaw config watch <git-url> [--interval-secs] [--require-signature]
  ironclaw harness install <path|git-url>
  ironclaw harness list [--json]
  ironclaw harness activate <id> --thread|--project|--tenant <scope>
  ironclaw harness deactivate --thread|--project|--tenant <scope>

This commit lands every one of those as a compiling, discoverable,
shell-completable subcommand in `ironclaw-reborn` today. Each is a
"stub" — it parses its full argument shape, validates types, then
fails with a structured error that names the operation, the tracking
issue (#3036), and the specific substrate it needs.

Why lock the surface now (not when the substrate lands):

  1. Shell completion / operator muscle memory: scripted tooling that
     discovers commands via `--help` or completion scripts sees the
     final command tree from day one. No "the command appeared in
     v0.X but didn't exist in v0.(X-1)" surprises.
  2. Argument shape is contractual: --dry-run, --lockfile, --git,
     --require-signature, --thread/--project/--tenant, --interval-secs
     are all decided here, on the PR that introduces the stubs, not
     six PRs later when implementation pressure tempts shortcuts.
  3. Forces the runtime-side admin contract to be designed up front:
     RebornAdminClient methods exist with their final signatures
     (sibling commit) because the CLI calls them. The substrate
     epic sub-issues just fill in bodies.

Structure:

  crates/ironclaw_reborn_cli/src/commands/
    config/                        # was flat config.rs
      mod.rs                       # dispatches to subcommands
      apply.rs                     # stub
      diff.rs                      # stub
      watch.rs                     # stub
    harness/                       # new
      mod.rs
      install.rs                   # stub
      list.rs                      # stub
      activate.rs                  # stub
      deactivate.rs                # stub
    stubs.rs                       # shared "not yet wired" output
                                   # helper -- one format across all
                                   # stubs so operators / scripts see
                                   # a uniform shape (operation name,
                                   # tracking issue, required substrate)

Behavior on invocation today:

  $ ironclaw-reborn config apply --dry-run /tmp/x
  ironclaw-reborn: command `config.apply` is not yet wired
    tracking issue : #3036
    requires       : BlueprintParser + BlueprintApplyService + typed repos
                     (Settings, Skill, Mission, Project)

  The command shape (arguments, output format) is locked from epic
  #3036; the substrate that makes it execute lands incrementally via
  the issue's sub-issues. Re-run after the relevant sub-issue ships.

  (exit code 1)

Smoke tests added (7 new tests, all passing):

  - help_lists_harness_subcommand_for_epic_3036
  - config_apply_is_a_discoverable_stub
  - config_diff_is_a_discoverable_stub
  - config_watch_is_a_discoverable_stub
  - harness_install_is_a_discoverable_stub
  - harness_list_is_a_discoverable_stub
  - harness_activate_is_a_discoverable_stub
  - harness_deactivate_is_a_discoverable_stub

All existing 35 CLI smoke tests continue to pass.
Adds `reborn_runtime_surface_is_futureproof_for_epic_3036` to the
Reborn dependency-boundary suite. Locks three properties:

  1. RebornRuntimeInput carries the explicit epic-3036 fields:
     api_version, identity, policy, drivers, harness. A regression
     that drops any of these (or removes the version stamp) fails
     the test alongside a precise message naming the missing field
     and the back-pointer to this rule.

  2. RebornAdminClient + RebornAdminScope + RebornAdminError exist
     on `ironclaw_reborn_composition`'s public surface, with every
     admin method epic #3036 specifies present as a typed async fn:
     apply_blueprint, diff_blueprint, install_harness,
     activate_harness, deactivate_harness, list_harnesses. The test
     reads the source file to assert presence (cheap and resilient;
     a smarter macro reflection isn't needed for a contract this
     stable).

  3. RebornAdminScope remains *sealed*: it carries a private
     SealedMarker field and outside crates cannot construct one
     without going through the published constructors. A regression
     that adds a third unconstrained `pub fn from_anything()` trips
     this assertion. The only legitimate public constructor today
     is `for_cli_admin(audit_reason)`; remote admin paths will
     gain their own constructor when the privilege/auth crate from
     epic #3036 lands.

  4. The CLI command tree contains the operator-facing stub files
     the epic enumerates:
       config/{mod, apply, diff, watch}.rs
       harness/{mod, install, list, activate, deactivate}.rs
       stubs.rs
     Removing any of these re-opens the operator-surface instability
     problem the stubs exist to prevent.
@github-actions github-actions Bot added size: XL 500+ changed lines risk: low Changes to docs, tests, or low-risk modules contributor: core 20+ merged PRs labels May 16, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements the foundational infrastructure for epic #3036, introducing a versioned input contract for the Reborn runtime, a privileged admin surface for configuration and harness management, and stubbed CLI commands for blueprint application and harness lifecycle operations. The review feedback highlights several areas for improvement: the need to use Weak references in the conversation registry to prevent memory leaks, and the necessity of enforcing mutual exclusivity for CLI arguments using clap's conflict attributes in several command definitions.

Comment on lines +204 to +206
conversations: tokio::sync::RwLock<
std::collections::HashMap<ThreadId, ConversationContext>,
>,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The conversations map is an unbounded HashMap that grows with every new conversation. To prevent memory leaks in long-running processes, store Weak references to the resources and prune entries where the Weak reference can no longer be upgraded on access, as per the repository's registry management guidelines.

References
  1. When using a static HashMap as a registry for shared resources like locks, store Weak references to the resources to prevent memory leaks. On access, prune entries where the Weak reference can no longer be upgraded.

Comment on lines +19 to +24
#[arg(value_name = "PATH", required_unless_present = "git")]
pub path: Option<PathBuf>,

/// Git URL of a blueprint repository. Mutually exclusive with `PATH`.
#[arg(long = "git", value_name = "URL")]
pub git: Option<String>,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The path and git arguments are documented as mutually exclusive, but this is not enforced by clap. While required_unless_present ensures at least one is provided, it does not prevent both from being specified simultaneously. Use conflicts_with to enforce mutual exclusivity at the argument level.

Suggested change
#[arg(value_name = "PATH", required_unless_present = "git")]
pub path: Option<PathBuf>,
/// Git URL of a blueprint repository. Mutually exclusive with `PATH`.
#[arg(long = "git", value_name = "URL")]
pub git: Option<String>,
#[arg(value_name = "PATH", required_unless_present = "git", conflicts_with = "git")]
pub path: Option<PathBuf>,
/// Git URL of a blueprint repository. Mutually exclusive with PATH.
#[arg(long = "git", value_name = "URL", conflicts_with = "path")]
pub git: Option<String>,

Comment on lines +18 to +29
#[arg(long = "thread", value_name = "THREAD_ID")]
pub thread: Option<String>,

/// Activate on a project. Mutually exclusive with `--thread` and
/// `--tenant`.
#[arg(long = "project", value_name = "PROJECT_ID")]
pub project: Option<String>,

/// Activate on a tenant. Mutually exclusive with `--thread` and
/// `--project`. Tenant-wide activation requires admin scope.
#[arg(long = "tenant", value_name = "TENANT_ID")]
pub tenant: Option<String>,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The --thread, --project, and --tenant arguments are documented as mutually exclusive, but this is not enforced by clap. Use conflicts_with_all to ensure only one scope is specified at a time, preventing ambiguous activation requests.

Suggested change
#[arg(long = "thread", value_name = "THREAD_ID")]
pub thread: Option<String>,
/// Activate on a project. Mutually exclusive with `--thread` and
/// `--tenant`.
#[arg(long = "project", value_name = "PROJECT_ID")]
pub project: Option<String>,
/// Activate on a tenant. Mutually exclusive with `--thread` and
/// `--project`. Tenant-wide activation requires admin scope.
#[arg(long = "tenant", value_name = "TENANT_ID")]
pub tenant: Option<String>,
#[arg(long = "thread", value_name = "THREAD_ID", conflicts_with_all = ["project", "tenant"])]
pub thread: Option<String>,
/// Activate on a project. Mutually exclusive with --thread and
/// --tenant.
#[arg(long = "project", value_name = "PROJECT_ID", conflicts_with_all = ["thread", "tenant"])]
pub project: Option<String>,
/// Activate on a tenant. Mutually exclusive with --thread and
/// --project. Tenant-wide activation requires admin scope.
#[arg(long = "tenant", value_name = "TENANT_ID", conflicts_with_all = ["thread", "project"])]
pub tenant: Option<String>,

Comment on lines +11 to +22
pub(crate) struct HarnessDeactivateCommand {
/// Deactivate on a thread.
#[arg(long = "thread", value_name = "THREAD_ID")]
pub thread: Option<String>,

/// Deactivate on a project.
#[arg(long = "project", value_name = "PROJECT_ID")]
pub project: Option<String>,

/// Deactivate on a tenant.
#[arg(long = "tenant", value_name = "TENANT_ID")]
pub tenant: Option<String>,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The --thread, --project, and --tenant arguments should be mutually exclusive for deactivation as well to ensure the command intent is clear. Use conflicts_with_all to enforce this constraint in clap.

    #[arg(long = "thread", value_name = "THREAD_ID", conflicts_with_all = ["project", "tenant"])]
    pub thread: Option<String>,

    /// Deactivate on a project.
    #[arg(long = "project", value_name = "PROJECT_ID", conflicts_with_all = ["thread", "tenant"])]
    pub project: Option<String>,

    /// Deactivate on a tenant.
    #[arg(long = "tenant", value_name = "TENANT_ID", conflicts_with_all = ["thread", "project"])]
    pub tenant: Option<String>,

@ilblackdragon

Copy link
Copy Markdown
Member

Code Review

Overview

Forward-architecture PR that reshapes RebornRuntimeInput / RebornRuntime / the CLI command tree to absorb epic #3036 ("Configuration-as-Code") without future shape changes. Every line is one of three things: a typed DTO that didn't exist, a stub method whose signature is committed by the epic, or a boundary test pinning the shape. No behavior change for an operator running ironclaw-reborn run today.

+2124 / −182, 23 files. Stacked on #3695.

Strengths

  • Shape-first discipline is enforced, not aspirational. New boundary test reborn_runtime_surface_is_futureproof_for_epic_3036 greps for each of the five futureproof fields on RebornRuntimeInput, every admin method on RebornAdminClient, the private _sealed: SealedMarker field on RebornAdminScope, and the ten CLI stub files. A future regression hits a precise CI failure with the rationale inline.
  • RebornAdminScope is sealed structurally, not by convention. Private unit struct SealedMarker lives inside the type; outside crates literally cannot construct it because they can't name the field. The only public ctor is for_cli_admin, with the future for_authenticated_admin(credential, ...) reserved for when the auth crate lands. Sealing pattern is clean and minimal.
  • Per-conversation identity is real, not faked. The previous single thread_scope is replaced by an Arc<RwLock<HashMap<ThreadId, ConversationContext>>>, and send_user_message_with_options looks up the per-conversation context to mint the right TurnScope. new_conversation_for(ConversationIdentity) actually composes a distinct ThreadScope. This is a meaningful capability addition that the stub framing slightly understates — it unblocks multi-tenant on day one.
  • RebornHarnessId validation is strict and tested. ASCII lowercase + digits + hyphens, must start with a lowercase letter, ≤64 bytes. Serde try_from = String so deserialization can't bypass new. Tests cover empty, Red-Team (invalid start), red_team (invalid char), 9-leading-digit, oversized. Mirrors PackageId discipline.
  • api_version fail-closed on major mismatch. Mandatory RebornRuntimeApiVersion field; major mismatch returns IncompatibleApiVersion; minor diffs are tolerated. Tests exercise both. Wire format ironclaw.runtime/v1.0 is stable.
  • Uniform stub command output. stubs::fail_not_yet_wired is the single source of stub-output formatting. Output names operation, tracking_issue, requires; greppable, scriptable; tests assert each command's specific operation string.
  • Per-DTO #[cfg(test)] mod tests blocks. Every new module ships its own tests in-file rather than dumping them in tests/runtime.rs. Focused, fast, easy to find.
  • Documentation is unusually thorough. Every module header has the same three sections: what it is, what it does today, what it does after epic [EPIC] Configuration-as-Code for IronClaw Reborn: tenant blueprints and use-case harnesses #3036 lands. Future maintainers can read these and understand the trajectory without consulting the epic.
  • #[non_exhaustive] on the right enums. RebornDriverChoice, HarnessActivationScope, ApplyChangeAction — all good. Lets future variants land non-breakingly.
  • Cross-PR test integrity. The integration test from arch(reborn): consolidate composition root, narrow public surface, ship live binary #3695 (stub_gateway_send_cancels_recovery_required_and_releases_conversation) is preserved without modification — the reshape didn't regress the live-agent path.

Issues / Suggestions

1. conversations map is unbounded (medium)

new_conversation and new_conversation_for insert into Arc<RwLock<HashMap<ThreadId, ConversationContext>>> with no eviction path. Long-running processes (anything beyond the CLI's short-lived sessions — a future server adapter, an e2e harness, a hosted runtime) will accumulate entries forever. Each entry is small (a tenant id, agent id, owner id, optional project, a ThreadScope), so the leak rate is modest, but it is unbounded.

Suggest exposing close_conversation(id: ConversationId) and removing the entry on shutdown. The shutdown removal is the easy first step.

2. RebornDriverChoice::Planned reachable from config but fails at boot (medium)

drivers.default == RebornDriverChoice::Planned yields RebornRuntimeError::InvalidArgument("...recognized but not yet wired end-to-end in build_reborn_runtime; track epic #3036 sub-issue..."). The shape is fine, but operators who set default = "planned" in #3704's config.toml get a runtime error rather than a parse-time error. Since the TOML side accepts the wire string, the CLI's reject_unsupported_runtime_sections (in #3704) should grow a check for drivers.default = "planned" — fail at parse time so operators see the issue before they've spun up the agent.

3. harness log noise on every boot and every send (low)

Two places emit tracing::warn!:

  • build_reborn_runtime when harness is Some ("recorded but not yet wired")
  • send_user_message_with_options when SendMessageOptions.harness_override is Some ("recorded but not yet wired")

Once operators start setting harness identifiers as a planning measure (which the PR description encourages with "operators free to populate mission ahead of time"), these warn lines fire on every send. Default log levels surface warns; production logs get cluttered with no actionable signal.

Suggest dropping these to debug! (the structured field stays in the log if anyone needs it) or warn-once-per-process by tracking emission in a OnceLock<()>.

4. RebornAdminScope.audit_reason is unvalidated String (low)

Construction accepts empty strings, garbage, or secret-shaped values without any check. The string is destined for audit logs (epic #3036's CapabilityHost::invoke_json invocation reason). Apply reject_inline_secret (already exported from ironclaw_reborn_config per #3704) to catch operators pasting credentials into audit reasons. At minimum, reject empty.

5. for_cli_admin is a misnomer (low)

The doc says "the future privilege/auth crate will add for_authenticated_admin", but today for_cli_admin is callable from anywhere the composition crate's public API is reachable — including channel adapters or web handlers if anyone wires them up. Sealing is structural via the marker field, but the name implies a CLI-scope restriction that isn't enforced. Future contributors reading "the CLI is the only legitimate caller today" comments will assume the function name encodes the restriction.

Suggest renaming to for_local_admin(...), unverified_admin(...), or for_legacy_unauthenticated_admin(...) so the misnomer doesn't ossify into an assumption.

6. RebornRuntimeError::InvalidArgument is overloaded (low)

Both "thread id is invalid" (validation failure) and "Planned driver is not yet wired" (unimplemented feature) use the same InvalidArgument variant. Boot-time-unimplemented vs. invalid-caller-input get conflated. Consider mirroring RebornAdminError::NotYetWired { operation, tracking_issue, requires } on the agent surface so the two are distinguishable. Today's stub callers can match on it cleanly.

7. BlueprintSource lacks #[non_exhaustive] (nit)

BlueprintSource::Path(...), GitUrl(...). Future likely variants: OCI registry, S3, signed URL. Mark #[non_exhaustive] to match the discipline applied to neighboring enums.

8. fail_not_yet_wired may emit duplicate error text (nit)

The function eprintln!s the structured fields, then returns Err(anyhow::anyhow!(error)). anyhow's default formatter prints error.to_string() again (which is RebornAdminError::NotYetWired's #[error("...")] line) — so a user sees the structured block and anyhow's rendering of the same content. Either:

  • Print only the structured form and return Err(anyhow::anyhow!("operation {operation} is not yet wired (see stderr)")), OR
  • Convert to a std::process::ExitCode::FAILURE without a second printout.

Worth a small polish for stderr cleanliness.

9. HarnessActivationScope variants carry untyped strings (nit)

Thread { thread_id: String }, Project { project_id: String }, Tenant { tenant_id: String }. Defensible to ship as strings now since the activation service isn't wired, but the typed variants (ThreadId, ProjectId, TenantId) already exist in ironclaw_host_api. Validating at the CLI boundary catches typos earlier and matches the discipline applied to RebornHarnessId.

10. with_default doesn't migrate the previous default into additional (nit)

RebornDriverConfig::with_additional skips duplicates against both default and additional. with_default overwrites default without checking what happens to the previous default. The chain config.with_default(TextOnly).with_additional(Planned).with_default(Planned) ends with default=Planned, additional=[Planned] — both. all_choices dedupes so no functional bug, but a maintainer reading the struct would expect with_default to either (a) keep additional in sync or (b) document that it's the caller's job. Add the comment.

11. ConversationIdentity::with_owner lacks test coverage (nit)

identity.rs test conversation_identity_builders_compose exercises for_tenant + with_agent + with_project. Missing with_owner. Trivial to add.

12. CLI stub tests don't pass env_clear() (nit)

Each smoke test sets IRONCLAW_REBORN_HOME and removes USERPROFILE, but the rest of the parent process env leaks in. The stubs short-circuit before any provider env lookup, so it's not a real issue today, but env_clear() would harden the tests against env-sensitive logic added later.

Project-conventions check

  • .unwrap() only in tests ✓
  • Comments are dense but on-target — each documents the trajectory (today vs. after substrate) not the code ✓
  • New crate dependencies: none — pure restructuring within ironclaw_reborn_composition ✓
  • Boundary tests pinned ✓
  • Sealing pattern (SealedMarker + private field) ✓

Risk summary

Low. This PR is genuinely additive — every existing code path either stays the same or gains an additional code path behind a feature/flag. The two items worth picking up before too many sub-issues land on top of this shape are (1) bounding the conversations map and (2) the Planned driver / TOML cross-validation.

Strong forward-architecture work. The combination of shape-locked stubs + boundary tests + uniform NotYetWired errors gives sub-issues a clean drop-in target.

— Posted by Claude Code review

@zmanian zmanian left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed write-up. The narrative is clear and I appreciate the table mapping epic #3036 concepts to today's seams. That said, I want to push back hard on the framing. "Futureproofing" is exactly the kind of work I'm most skeptical of, and the PR-as-shipped bakes in a lot of shape we may regret.

What is load-bearing today and worth keeping

  • RebornIdentityConfig + ConversationIdentity + new_conversation_for(...) + the conversations HashMap in RebornRuntime. This is real, solves a real problem (the old code hardcoded tenant_id = "reborn-cli" and shared one ThreadScope across all conversations), and the refactor of send_user_message to look up per-conversation context is a genuine fix, not speculation. Keep this.
  • The deletion of actor_user_id and thread_scope fields on RebornRuntime in favor of identity-driven scope construction is also good — that was a latent multi-tenant bug.

Cut those out and ship them on their own. The rest concerns me.

Speculative surface I'd push back on

  1. RebornHarnessSelection / RebornHarnessId / SendMessageOptions::harness_override. Zero consumers today. build_reborn_runtime logs a warn! and ignores it. send_user_message_with_options logs a warn! and ignores it. We are committing to (a) the harness-id stringly-typed namespace, (b) per-message override being the activation granularity (vs. per-thread or per-tenant), and (c) the name "harness" — all before the harness substrate exists and can talk back about what shape it actually wants. When sub-issue 4 from #3036 lands, the harness service will have opinions about activation scope that may not match this DTO. Better to add this field when the substrate lands and we know the shape, in one PR. Removing a no-op field later is cheap; reshaping a public field with consumers is expensive.

  2. RebornDriverChoice::Planned. This is a variant that, if selected, returns Err(InvalidArgument) with a "not yet wired" message. So the only valid value today is TextOnly. A Vec<RebornDriverChoice> of additional drivers with one possible value, plus a default field, is solving zero problems today. When planned driver lands (#3651 — a separate slice), it brings its own host wiring requirements that may or may not fit this DTO. Drop RebornDriverChoice::Planned and RebornDriverConfig entirely; add a driver-config DTO at the point planned-driver actually wires.

  3. RebornPolicyConfig. All three fields (deployment_mode, default_profile, default_approval_policy) hit let _ = (&policy, &drivers); and a tracing::debug!. Nothing reads them at request time. The comment honestly admits "field-level enforcement is downstream work." Same argument: add the input when there's a consumer. Today this is shape-bikeshedding without a tiebreaker — the RuntimePolicyRepo lookup path doesn't exist, so we're guessing what input it wants.

  4. RebornAdminClient + every DTO under it. Six async methods, all returning Err(NotYetWired { tracking_issue: "#3036" }). ApplyBlueprintRequest, DiffBlueprintRequest, InstallHarnessRequest, ActivateHarnessRequest, DeactivateHarnessRequest, HarnessActivationScope, HarnessDescriptor, ApplyMode, ApplyReport, ApplyChange, ApplyChangeAction, BlueprintSource — every one of these is exported, every one is a guess at a shape the substrate will dictate. The PR description acknowledges this: "the parsed-blueprint DTO will live in the future ironclaw_blueprint crate ... For now this is a path/url placeholder so the admin surface already takes a typed argument and won't break when the parser lands." That's not stability, that's saving on one rename in a single PR while paying for it with ~400 lines of dead public API right now.

  5. RebornAdminScope sealing. I like the direction — privilege gating belongs here — but the SealedMarker mechanism does nothing today. Any caller with the type in scope can call RebornAdminScope::for_cli_admin("whatever"). The string audit_reason has no enforcement. The seal only prevents non-pub field construction, and the only pub constructor accepts any string. This is the ergonomic shape of a privilege gate without the actual gating. When the auth crate lands, the constructor changes anyway, so this is essentially comment-driven design. Either ship it with real auth (credential parameter on for_cli_admin, even if the only valid credential today is "I'm the CLI binary") or wait.

  6. reborn_runtime_surface_is_futureproof_for_epic_3036 boundary test. This test reads source files as strings and assert!(contains(...)) on struct field names and the existence of CLI stub files. I understand the intent — pin the surface so a contributor can't drift back to implicit shape — but it pins today's guess at the surface, and it pins it by string match on source. Any reshape (which sub-issue 2/3/4 may demand) trips this test and the contributor either modifies the test (rendering it ceremonial) or argues with it. A test like this only makes sense when the shape is settled by something external (an RFC, a wire format, an upstream API). Epic #3036 sub-issues haven't been written; the shape isn't settled. Drop this test or scope it tightly to the load-bearing identity layer.

  7. The CLI stub commands. I'm more sympathetic here — harness install/list/activate/deactivate + config apply/diff/watch as discoverable stubs that point operators to the tracking issue is reasonable UX. But: shell completion as justification is thin (operator muscle memory has nothing to anchor on yet either), and locking the flag shape of these commands (--thread/--project/--tenant, --require-signature, --interval-secs) is the same kind of guess as #4. If you keep the stubs, drop the flags and accept positional args + free-form ones until the substrate constrains them.

On the "concentrate the reshape" argument

The PR description argues this is cheaper than five future breaking changes. I'd argue the opposite. Today's PR commits to specific DTO shapes for 5+ subsystems whose substrate doesn't exist. Each substrate sub-issue will discover real constraints — what the parser actually returns, what the harness service actually needs to filter on, what the policy repo's per-request resolver actually keys off — and at that point either (a) the DTO matches by luck, or (b) we change it. Option (b) is the same breaking change we were trying to avoid, just earlier and with less information.

The cheapest version of futureproofing is not doing it, and adding each input field at the same PR that adds its consumer. That gives us tiebreakers (the consumer's API) instead of vibes (what we think the consumer's API will probably look like).

What I'd ship instead

Split this into:

  1. Identity refactor (~300 LOC): RebornIdentityConfig, ConversationIdentity, new_conversation_for, conversations map, removal of hardcoded reborn-cli tenant. Ship this now, it's a real fix.
  2. Api-version stamp (optional, ~50 LOC): If you want the version-stamp discipline, add it minimally — one variant, mandatory field, fail-closed on major mismatch. Skip the RebornRuntimeApiVersionError enum elaboration until there's a second variant.
  3. Everything else: defer to the sub-issue that adds the consumer.

Substrate/binary boundary

To answer the explicit question: no, this PR doesn't entangle binary concerns into substrate. Everything new lives in ironclaw_reborn_composition and the CLI crate. The substrate crates (ironclaw_reborn, ironclaw_turns, ironclaw_host_api) are not touched. That much is clean.

Bottom line

REQUEST CHANGES. The identity refactor is a clear win; please carve it out. The rest is too much shape committed too early on epic #3036 contracts that don't exist yet. I'd rather take five smaller breaking-change PRs that each follow their consumer than one large PR that pre-commits to all the shapes.

— Zaki

Base automatically changed from reborn/composition-runtime-and-narrow-surface to reborn-integration May 17, 2026 18:20
Base automatically changed from reborn-integration to main June 5, 2026 12:57
@serrrfirat serrrfirat closed this Jun 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor: core 20+ merged PRs risk: low Changes to docs, tests, or low-risk modules size: XL 500+ changed lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants