lywsvip/ironclaw
1
0
Fork 0
mirror of https://github.com/nearai/ironclaw.git synced 2026-09-03 08:06:01 +08:00
Code Issues Packages Projects Releases Wiki Activity

fix(skills): stop rejecting inline bundle installs and stop dropping url conflicts

Browse Source 
Review finding on #7141, verified against `dispatch_install` before acting.
Two defects in `resolve_install_input`, in opposite directions:

1. Inline installs lost their bundle. The inline arm required `files`,
   `source` and `source_url` to be ABSENT, so `{name, content, files}` fell
   through to `InputEncode`. That shape is fully supported downstream —
   `dispatch_install` reads `content` and then `parse_install_files`,
   `parse_install_source` and `source_url` off the same object — so a valid
   bundle install was rejected before it ever reached the dispatcher. Those
   three keys conflict with `url`, not with `content`.

2. URL installs silently discarded conflicts. The url arm accepted `url`
   even when `files`/`source`/`source_url` were present, then rebuilt a fresh
   object from the fetched payload — so those fields vanished without a word
   and the caller saw a successful install of something it had not asked for.
   The function's own contract already called that combination an input error
   ("`url` combined with `files`/`source`/`source_url`"); now the code agrees.

Sabotage-tested both guards, and the second round caught a defect in the TEST
rather than the code — worth recording, because it is the failure mode this
program keeps hitting:

  * inline arm made over-strict again: RED on
    `inline_install_keeps_its_bundle_files_source_and_source_url`.
  * url conflict guard removed: initially STILL GREEN. The test used
    `https://example.test/...`, an unroutable host that `validate_skill_url`
    rejects with the SAME `InputEncode` kind — so it passed whether or not the
    guard existed. Rewritten against an allowed `raw.githubusercontent.com`
    URL, where removing the guard now reaches the fetch and fails
    `NetworkDenied`: RED, "left: NetworkDenied, right: InputEncode". The test
    also asserts `usage() == None`, since the guard must reject before any
    egress is consumed.
  * restored: 112 passed, 0 failed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
BenKurrek
2026-08-04 09:13:27 -04:00
parent ba79cb6910
commit b57ac8e59f
1 changed files with 103 additions and 8 deletions
Download Patch File Download Diff File Expand all files Collapse all files

111
crates/extensions/ironclaw_extension_support/src/skills.rs
View File

@@ -127,14 +127,27 @@ pub async fn resolve_install_input(
.get("url")
.and_then(Value::as_str)
.filter(|value| !value.trim().is_empty());
// `files`, `source` and `source_url` are companions of an *inline* install,
// not competitors: `dispatch_install` reads `content` and then
// `parse_install_files`/`parse_install_source`/`source_url` from the same
// object, so `{content, files}` is a bundle install it fully supports.
// Guarding the inline arm on their absence rejected that valid shape with
// `InputEncode` before it ever reached the dispatcher. They are conflicting
// only against `url`, which is what this function's contract says and what
// the url arm below now enforces.
let has_url_conflict = object.contains_key("files")
|| object.contains_key("source")
|| object.contains_key("source_url");
match (has_content, url) {
(true, None)
if !object.contains_key("files")
&& !object.contains_key("source")
&& !object.contains_key("source_url") =>
{
Ok(input.clone())
}
(true, None) => Ok(input.clone()),
// Reject rather than silently drop. The rewrite below builds a fresh
// object from the fetched payload, so any `files`/`source`/`source_url`
// the caller sent alongside `url` would be discarded without a word —
// the caller would see a successful install of something other than
// what it asked for.
(false, Some(_)) if has_url_conflict => Err(SkillManagementCapabilityError::new(
RuntimeDispatchErrorKind::InputEncode,
)),
(false, Some(url)) => {
let payload = url_install::fetch_skill_url_payload(fetch, url, usage).await?;
let mut rewritten = Map::new();
@@ -515,7 +528,7 @@ mod tests {
use ironclaw_filesystem::InMemoryBackend;
use ironclaw_host_api::{
ids::{InvocationId, UserId},
ids::{CapabilityId, InvocationId, UserId},
mount::MountView,
resource::ResourceScope,
};
@@ -542,4 +555,86 @@ mod tests {
assert_eq!(error.kind(), RuntimeDispatchErrorKind::InputEncode);
}
/// A fetch context with no egress, which must never be used.
///
/// Both cases below are decided from the input shape alone, so reaching the
/// network at all would itself be the bug — and with `runtime_http_egress:
/// None` a fetch could not succeed anyway, so a regression that started
/// taking the url arm fails loudly here instead of going quiet.
fn unused_fetch_context() -> SkillUrlFetchContext {
SkillUrlFetchContext {
capability_id: CapabilityId::new("ironclaw.skill.install").unwrap(),
scope: ResourceScope::local_default(UserId::new("alice").unwrap(), InvocationId::new())
.unwrap(),
runtime_http_egress: None,
}
}
#[tokio::test]
async fn inline_install_keeps_its_bundle_files_source_and_source_url() {
// Regression: the inline arm used to require that `files`, `source`
// and `source_url` were all absent, so this shape — which
// `dispatch_install` fully supports, reading `content` and then
// `parse_install_files` / `parse_install_source` / `source_url` off the
// same object — was rejected with `InputEncode` before it ever reached
// the dispatcher. They conflict with `url`, not with `content`.
let input = json!({
"name": "bundled",
"content": "# SKILL\n",
"files": [{"path": "a.txt", "bytes_base64": "aGk="}],
"source": "installed_url",
"source_url": "https://example.test/skill",
});
let mut usage = ResourceUsage::default();
let resolved = resolve_install_input(&input, &unused_fetch_context(), &mut usage)
.await
.expect("an inline bundle install is a valid shape");
// Passed through untouched — the resolver has nothing to resolve here.
assert_eq!(resolved, input);
}
#[tokio::test]
async fn url_install_rejects_conflicting_fields_instead_of_dropping_them() {
// The url arm rebuilds a fresh object from the fetched payload, so
// anything the caller sent beside `url` would be silently discarded and
// the caller would see a successful install of something it did not ask
// for. The contract calls that an input error; now the code does too.
//
// The URL must be a *valid, allowed* skill host. An unroutable host is
// rejected by `validate_skill_url` with the same `InputEncode` kind, so
// a test written against one passes whether or not the conflict guard
// exists — it was, and it did, until sabotage-testing caught it. With an
// allowed host and no egress configured, removing the guard makes this
// reach the fetch and fail `Backend` instead, so the assertion below
// genuinely discriminates.
let allowed_url =
"https://raw.githubusercontent.com/Pika-Labs/Pika-Skills/main/helper/SKILL.md";
for conflicting in ["files", "source", "source_url"] {
let mut object = serde_json::Map::new();
object.insert("url".to_string(), Value::String(allowed_url.to_string()));
object.insert(conflicting.to_string(), json!("whatever"));
let input = Value::Object(object);
let mut usage = ResourceUsage::default();
let error = resolve_install_input(&input, &unused_fetch_context(), &mut usage)
.await
.expect_err(conflicting);
assert_eq!(
error.kind(),
RuntimeDispatchErrorKind::InputEncode,
"url + {conflicting} must be rejected by the guard, not dropped \
(a `Backend` kind here means the guard is gone and the fetch ran)"
);
// The guard rejects before any egress, so nothing was consumed.
assert_eq!(
error.usage(),
None,
"{conflicting} must not reach the fetch"
);
}
}
}