Skip to content

fix(core): harden runtime contract invariants #24

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
AjayThorve wants to merge 1 commit into from
Closed

fix(core): harden runtime contract invariants #24

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
99 changes: 92 additions & 7 deletions .coderabbit.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,24 +2,109 @@
# SPDX-License-Identifier: Apache-2.0

# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json
# CodeRabbit configuration: https://docs.coderabbit.ai/reference/configuration
#
# Intentionally minimal: NeMo Fabric is an early POC and the codebase is still
# changing, so this avoids per-path rules and strict merge gates. Tighten as the
# contract stabilizes.
# Docs: https://docs.coderabbit.ai/reference/configuration
language: "en-US"
tone_instructions: "Be concise and technical. Prioritize correctness and maintainability over style nits."
tone_instructions: "Be concise, technical, and specific. Prioritize correctness, safety, and maintainability over style nits."

reviews:
profile: chill
profile: assertive
review_status: true
review_details: true
collapse_walkthrough: false
request_changes_workflow: false
pre_merge_checks:
title:
mode: error
requirements: >-
Title must follow Conventional Commits format:
type(optional-scope)[!]: concise imperative summary

Allowed types: feat, fix, docs, style, refactor, perf, test, build, ci, chore, revert.
Use lowercase type and scope. If the change is a breaking change, add a ! suffix before the colon.
Keep it under 72 characters and do not use a trailing period.
description:
mode: warning
issue_assessment:
mode: warning
auto_title_placeholder: "@coderabbitai"
auto_title_instructions: |
Generate the PR title using Conventional Commits format:
type(optional-scope)[!]: concise imperative summary

Allowed types: feat, fix, docs, style, refactor, perf, test, build, ci, chore, revert.
Prefer scopes from this repository when clear: core, cli, python, schemas, adapters, docs, ci, deps
Use lowercase type and scope. If the change is a breaking change, add a ! suffix before the colon.
Keep it under 72 characters and do not use a trailing period.
auto_review:
base_branches: ["main", "release/.*"]
enabled: true
drafts: false
auto_incremental_review: true
path_filters:
- "!target/**"
- "!**/target/**"
- "!.venv/**"
- "!**/.venv/**"
- "!**/node_modules/**"
- "!**/.pytest_cache/**"
- "!**/.ruff_cache/**"
- "!.tmp/**"
- "!docs/python-sdk/**"
path_instructions:
- path: "crates/fabric-core/src/**/*.rs"
instructions: |
Review the Rust core for runtime lifecycle correctness, handle validation, capability routing accuracy, schema stability, and error semantics.
Public API changes should preserve existing behavior unless tests and docs show the intended migration path.
- path: "crates/fabric-python/**/*"
instructions: |
Treat native binding changes as public API changes. Check JSON/type conversion, error propagation, GIL/thread behavior, and parity with the Python SDK.
- path: "python/src/nemo_fabric/**/*"
instructions: |
Review Python SDK changes for typed API consistency, import-time dependency neutrality, async/session behavior, and parity with the native extension.
Stubs and runtime implementations should stay aligned.
- path: "schemas/**/*"
instructions: |
Schemas are generated public contract snapshots. Check that schema diffs correspond to intentional Rust type changes and are covered by core tests.
- path: "{tests/**,python/tests/**}"
instructions: |
Tests should cover the behavior promised by the changed API surface, including error paths, lifecycle cleanup, and SDK/native parity where relevant.
- path: "{adapters/**,examples/**}"
instructions: |
Review adapter and example changes for command correctness, config/schema consistency, artifact handling, and compatibility with the public Fabric contracts.
- path: "{docs/**,README.md,AGENTS.md}"
instructions: |
Review documentation for technical accuracy against the current API, command correctness, and consistency with generated schemas.
- path: "**/SKILL.md"
instructions: |
Do not flag SKILL.md files for missing SPDX headers. Skill entrypoints intentionally start with YAML frontmatter instead.
Verify that every SKILL.md keeps valid YAML frontmatter with at least name and description fields before the Markdown body.
poem: false
sequence_diagrams: true
suggested_reviewers: false

tools:
clippy:
enabled: true
ruff:
enabled: true
shellcheck:
enabled: true
yamllint:
enabled: true
markdownlint:
enabled: true
gitleaks:
enabled: true
osvScanner:
enabled: true
semgrep:
enabled: true

knowledge_base:
code_guidelines:
enabled: true
filePatterns:
- "AGENTS.md"
- "CLAUDE.md"
- "CONTRIBUTING.md"
- ".agents/skills/**/*.md"
123 changes: 102 additions & 21 deletions crates/fabric-core/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1070,7 +1070,8 @@ fn resolve_capability_plan(
.unwrap_or_default();
let tools_are_native = config.tools.is_some() && accepts("tools");
let mut native = CapabilityTargetPlan::default();
let mut managed = CapabilityTargetPlan::default();
let managed = CapabilityTargetPlan::default();
let mut unsupported = CapabilityTargetPlan::default();
let mut routes = Vec::new();

if config.tools.is_some() {
Expand All @@ -1083,12 +1084,12 @@ fn resolve_capability_plan(
reason: "selected adapter accepts Fabric tools config".to_string(),
});
} else {
managed.tools_configured = true;
unsupported.tools_configured = true;
routes.push(CapabilityRoute {
kind: CapabilityKind::Tools,
name: "tools".to_string(),
target: CapabilityTarget::FabricManaged,
reason: "selected adapter does not declare native tools support".to_string(),
target: CapabilityTarget::Unsupported,
reason: "selected adapter does not declare native tools support and Fabric-managed tools are not implemented".to_string(),
});
}
}
Expand All @@ -1103,12 +1104,12 @@ fn resolve_capability_plan(
reason: "selected adapter accepts Fabric skills config".to_string(),
});
} else {
managed.skill_paths = skill_paths.clone();
unsupported.skill_paths = skill_paths.clone();
routes.push(CapabilityRoute {
kind: CapabilityKind::Skills,
name: "skills".to_string(),
target: CapabilityTarget::FabricManaged,
reason: "selected adapter does not declare native skills support".to_string(),
target: CapabilityTarget::Unsupported,
reason: "selected adapter does not declare native skills support and Fabric-managed skills are not implemented".to_string(),
});
}
}
Expand All @@ -1128,16 +1129,16 @@ fn resolve_capability_plan(
),
});
} else {
managed.mcp_servers.insert(name.clone(), server.clone());
unsupported.mcp_servers.insert(name.clone(), server.clone());
routes.push(CapabilityRoute {
kind: CapabilityKind::Mcp,
name: name.clone(),
target: CapabilityTarget::FabricManaged,
target: CapabilityTarget::Unsupported,
reason: match server.exposure {
McpExposure::FabricManaged => {
"MCP server explicitly requests Fabric-managed exposure".to_string()
"MCP server explicitly requests Fabric-managed exposure but Fabric-managed MCP is not implemented".to_string()
}
_ => "selected adapter does not declare native MCP support".to_string(),
_ => "selected adapter does not declare native MCP support and Fabric-managed MCP is not implemented".to_string(),
},
});
}
Expand All @@ -1149,6 +1150,7 @@ fn resolve_capability_plan(
mcp_servers,
native,
managed,
unsupported,
routes,
}
}
Expand Down Expand Up @@ -1295,6 +1297,9 @@ pub struct CapabilityPlan {
/// Capabilities that Fabric must expose or manage outside the native harness config.
#[serde(default)]
pub managed: CapabilityTargetPlan,
/// Capabilities that are configured but not executable by this Fabric build.
#[serde(default)]
pub unsupported: CapabilityTargetPlan,
/// Routing decisions made while resolving the effective config.
#[serde(default, skip_serializing_if = "Vec::is_empty")]
pub routes: Vec<CapabilityRoute>,
Expand Down Expand Up @@ -1347,6 +1352,8 @@ pub enum CapabilityTarget {
HarnessNative,
/// Fabric exposes or manages the capability around the harness.
FabricManaged,
/// Capability is configured but no executable surface exists.
Unsupported,
}

/// Resolved MCP server exposure.
Expand Down Expand Up @@ -1559,18 +1566,17 @@ environment:
Some(1)
);
assert!(plan.capability_plan.native.mcp_servers.is_empty());
assert!(plan.capability_plan.managed.mcp_servers.is_empty());
assert!(
plan.capability_plan
.managed
.mcp_servers
.contains_key("github")
plan.capability_plan.routes.iter().any(
|route| route.name == "github" && route.target == CapabilityTarget::Unsupported
)
);
assert!(
plan.capability_plan
.routes
.iter()
.any(|route| route.name == "github"
&& route.target == CapabilityTarget::FabricManaged)
.unsupported
.mcp_servers
.contains_key("github")
);
}

Expand All @@ -1594,9 +1600,10 @@ environment:
.map(|telemetry| telemetry.relay_enabled),
Some(true)
);
assert!(plan.capability_plan.managed.mcp_servers.is_empty());
assert!(
plan.capability_plan
.managed
.unsupported
.mcp_servers
.contains_key("github")
);
Expand All @@ -1620,15 +1627,89 @@ environment:
assert_eq!(plan.profiles, vec!["mcp_github"]);
assert!(plan.config_path.ends_with("agent.yaml"));
assert!(plan.config.profiles.directories.is_empty());
assert!(plan.capability_plan.managed.mcp_servers.is_empty());
assert!(
plan.capability_plan
.managed
.unsupported
.mcp_servers
.contains_key("github")
);
assert_eq!(plan.config_root, root);
}

#[test]
fn unsupported_capabilities_do_not_claim_fabric_managed_execution() {
let root = std::env::temp_dir().join(format!(
"fabric-unsupported-capability-test-{}",
std::process::id()
));
let _ = std::fs::remove_dir_all(&root);
std::fs::create_dir_all(root.join("adapters/minimal")).expect("create adapters");
std::fs::create_dir_all(root.join("skills/review")).expect("create skills");
std::fs::write(
root.join("agent.yaml"),
r#"schema_version: fabric.agent/v1alpha1
metadata:
name: unsupported-capability-agent
harness:
adapter_id: acme.fabric.minimal
models:
default:
provider: test
model: test-model
runtime:
mode: oneshot
transport: cli
input_schema: text
output_schema: text
tools:
- name: shell
skills:
paths:
- ./skills/review
mcp:
servers:
github:
transport: streamable-http
url: http://example.invalid/mcp
exposure: fabric_managed
"#,
)
.expect("write agent config");
std::fs::write(
root.join("adapters/minimal/fabric-adapter.json"),
r#"{
"adapter_id": "acme.fabric.minimal",
"adapter_kind": "process"
}"#,
)
.expect("write adapter descriptor");

let plan = resolve_run_plan(&root, None).expect("run plan");

assert!(!plan.capability_plan.managed.tools_configured);
assert!(plan.capability_plan.managed.skill_paths.is_empty());
assert!(plan.capability_plan.managed.mcp_servers.is_empty());
assert!(plan.capability_plan.unsupported.tools_configured);
assert_eq!(plan.capability_plan.unsupported.skill_paths.len(), 1);
assert!(
plan.capability_plan
.unsupported
.mcp_servers
.contains_key("github")
);
assert!(
plan.capability_plan
.routes
.iter()
.all(|route| route.target == CapabilityTarget::Unsupported),
"{:?}",
plan.capability_plan.routes
);

let _ = std::fs::remove_dir_all(root);
}

#[test]
fn later_profiles_override_earlier_profiles() {
let profiles = vec!["env_opensandbox".to_string(), "env_local".to_string()];
Expand Down
Loading