Repository navigation
feat(reborn): add extension manifest registry - #3015
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces several new crates to the IronClaw Reborn project, including ironclaw_approvals, ironclaw_authorization, ironclaw_events, ironclaw_extensions, and ironclaw_run_state, along with updates to the filesystem service. These changes establish the core infrastructure for approval resolution, capability authorization, event logging, and run-state management. I have reviewed the code and identified unused imports in crates/ironclaw_authorization/src/lib.rs and crates/ironclaw_run_state/src/lib.rs that should be removed to maintain clean code.
| use std::{ | ||
| collections::HashMap, | ||
| sync::{Mutex, MutexGuard}, | ||
| }; |
| use std::{ | ||
| collections::HashMap, | ||
| sync::{Mutex, MutexGuard}, | ||
| }; |
767a8c3 to
d20b0ff
Compare
| args: Vec<String>, | ||
| url: Option<String>, | ||
| }, | ||
| FirstParty { |
There was a problem hiding this comment.
High Severity — privileged runtime kinds can still be self-asserted by manifests.
TrustClass deserialization correctly rejects trust = "first_party" / "system", but this private RawRuntime enum reintroduces FirstParty and System as deserializable manifest variants. A manifest with trust = "sandbox" and [runtime] kind = "system" currently parses successfully and produces a descriptor with RuntimeKind::System.
That violates the host-assigned-only runtime invariant and means future dispatcher/runtime wiring could route a user-supplied extension into a privileged execution lane. The existing test misses this because it sets both privileged trust and privileged runtime, so it fails on the trust field before exercising runtime parsing.
Please reject RawRuntime::FirstParty / RawRuntime::System in into_runtime() (or remove them from the manifest-facing enum and keep a separate trusted host-only constructor), and add tests where trust = "sandbox" is paired with kind = "first_party" / "system".
| if let Some(url) = &url { | ||
| validate_non_empty("mcp url", url)?; | ||
| } | ||
| Ok(ExtensionRuntime::Mcp { |
There was a problem hiding this comment.
Medium Severity — MCP runtime manifests can omit their endpoint entirely.
This branch only checks that transport is non-empty and that optional command / url values are non-empty when present. It still accepts manifests such as:
[runtime]
kind = "mcp"
transport = "stdio"That registers an MCP capability descriptor with no command, URL, or other usable endpoint, pushing a deterministic manifest validation error into a later runtime failure. It also allows ambiguous shapes such as stdio with a URL or both command and url.
Please make the transport contract structural: for example stdio requires exactly command (+ optional args) and no URL; HTTP/SSE transports require exactly url and no command. Add negative tests for missing endpoint, both endpoint forms, and transport/endpoint mismatch.
| } | ||
| } | ||
|
|
||
| fn looks_like_windows_path(value: &str) -> bool { |
There was a problem hiding this comment.
Medium Severity — Windows drive-relative asset paths pass containment validation.
looks_like_windows_path() only rejects drive paths when the colon is followed by / or \\, so a manifest module like C:evil.wasm is accepted as an extension asset path. That is still a Windows drive-relative path, not a normal manifest-local POSIX segment.
Because asset paths are later resolved into virtual paths and filesystem backends eventually push path segments into PathBuf, allowing drive prefixes weakens the “asset path stays under the extension root” contract on Windows targets.
Please reject any drive-prefix pattern like ^[A-Za-z]: (or simply reject : in asset path segments) and add a regression case for C:evil.wasm alongside the existing C:/... / backslash-style path cases.
| entries.sort_by(|left, right| left.name.cmp(&right.name)); | ||
|
|
||
| let mut registry = ExtensionRegistry::new(); | ||
| for entry in entries { |
There was a problem hiding this comment.
Medium Severity — a stray non-extension file aborts the whole discovery pass.
Discovery iterates every direct child returned by fs.list_dir(root) and immediately treats the name as an extension id plus directory containing manifest.toml. A single non-directory file under /system/extensions (for example .DS_Store, README.md, or a temporary editor file) makes discovery fail before any valid extension can be registered.
DirEntry already includes file_type, so this can fail more gracefully. Please either skip non-directory entries or return a targeted invalid-package error that does not prevent unrelated valid extension directories from being discovered. Add a test with a valid extension directory plus a stray file in the root.
|
Posted follow-up fixes in Changes added on top of the previous hardening commit:
Local verification passed:
|
henrypark133
left a comment
There was a problem hiding this comment.
Review: tighten extension registry contracts
The new extension crate is well-scoped: it keeps manifest parsing and descriptor extraction separate from execution, secret resolution, MCP connection, and resource reservation. The negative-path contract tests are also strong around unsafe asset paths, duplicate IDs, privileged runtime self-assertion, and filesystem discovery.
What looks good:
- The implementation keeps the extension registry as declarative metadata only, which matches the Reborn ownership boundary.
- Manifest validation rejects host paths, URLs, dot segments, malformed MCP runtime shapes, and self-asserted first-party/system runtimes.
- The crate-level and architecture tests cover the intended boundary without pulling in runtime execution dependencies.
Concerning: capability iteration is nondeterministic
ExtensionRegistry::capabilities() currently returns self.capabilities.values() from a HashMap. Discovery sorts extension directories and the registry preserves extension order, but capability iteration can still reorder across process runs. Once this registry feeds capability projection, approval displays, replay snapshots, or prompt/action lists, that nondeterminism can create noisy diffs and unstable runtime behavior. Please preserve capability order explicitly, either by iterating extension_order and each package's manifest-order capabilities or by maintaining a dedicated capability_order alongside the lookup map.
Concerning: registry insertion accepts descriptors that did not pass package validation
ExtensionPackage is fully public, and host-side callers will need to construct packages directly for first-party/system runtimes because those cannot come from manifest parsing. In that path, ExtensionRegistry::insert() only checks duplicate IDs and provider. It will accept descriptors that were added or mutated after validation even if they no longer match package.manifest, for example different runtime/trust/default-permission metadata or entirely undeclared capabilities. The registry then becomes the source of truth for authority metadata that never passed the manifest/package contract. Please re-validate descriptor-to-package/manifest consistency in insert() before accepting the package.
|
Addressed review 4191808728 in Changes:
Verification:
|
There was a problem hiding this comment.
Pull request overview
Adds the new ironclaw_extensions crate to the Reborn stack, providing a manifest/registry substrate for discovering and validating extension packages and extracting capability descriptors.
Changes:
- Introduces
ironclaw_extensionswith manifest parsing/validation, package/root checks, asset-path containment rules, and an in-memory registry. - Adds filesystem-backed discovery that scans
/system/extensions/<extension>/manifest.tomland builds a deterministic registry. - Adds contract-style tests covering valid/invalid manifests, registry duplication rules, and discovery behavior; wires the crate into the workspace.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/ironclaw_extensions/src/lib.rs | Implements manifest/runtime models, validation, registry, and filesystem-backed discovery. |
| crates/ironclaw_extensions/tests/extension_contract.rs | Contract tests for manifest parsing/validation, package checks, registry semantics, and discovery. |
| crates/ironclaw_extensions/Cargo.toml | Defines the new crate and its dependencies/dev-dependencies. |
| Cargo.toml | Adds crates/ironclaw_extensions to workspace members. |
| Cargo.lock | Records the new workspace package and its dependency edges. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let Some(extension_id) = root.as_str().strip_prefix("/system/extensions/") else { | ||
| return Err(invalid_package_root(root)); | ||
| }; | ||
| if extension_id.is_empty() || extension_id.contains('/') { | ||
| return Err(invalid_package_root(root)); | ||
| } |
There was a problem hiding this comment.
validate_asset_path checks value.contains('…') || value.chars().any(char::is_control). The explicit NUL check is redundant (NUL is a control char) and the literal NUL character is hard to spot in diffs/reviews. Consider removing the contains clause (or using \0 like crates/ironclaw_host_api/src/path.rs:230) and relying on the control-char scan.
* feat(reborn): add extension manifest registry * fix(reborn): harden extension manifest validation * fix: address review findings (iteration 1) * fix: address review findings (iteration 1) * fix: address extension registry review
Summary
Carves the deferred extension manifest/registry slice from the Reborn stack.
This is the smallest useful next PR after the currently open Reborn substrate/control PRs because
ironclaw_extensionsdepends only on:and unblocks later dispatcher/runtime/capability-host slices without pulling those crates forward.
Adds
crates/ironclaw_extensionswith:/system/extensionsFirstParty/Systemtrust/runtime classes via host-api serde rulesStacking note
This PR is draft/stacked on #2999 because it is meant to land after the currently open Reborn PRs:
After those merge into
reborn-integration, rebase this branch so the diff narrows to only:Scope boundary
This PR intentionally does not include:
Exposure checklist
TDD note
Copied the extension contract tests first against a RED stub and confirmed
cargo test -p ironclaw_extensionsfailed due missing manifest/registry types before porting the implementation.Verification
Passed:
Boundary grep returned no forbidden normal Reborn dependencies.
Refs #2987.