Repository navigation
feat(reborn): add script and mcp runtime lanes - #3027
Conversation
github-actions
Bot
added
scope: db/postgres
github-actions
Bot
added
risk: medium
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a major architectural update for IronClaw Reborn, adding ten new crates that implement core services for authorization, process lifecycle management, and scoped filesystem access. The review feedback focuses on enhancing system reliability and error handling. Key recommendations include explicitly logging errors in background process tasks to prevent silent failures and replacing brittle string-based 'not found' error detection with a dedicated NotFound variant in the FilesystemError enum for better type safety and consistency across the new crates.
| tokio::spawn(async move { | ||
| match executor.execute(request).await { | ||
| Ok(result) => { | ||
| if let Ok(record) = store.complete(&scope, process_id).await | ||
| && let Some(result_store) = &result_store | ||
| { | ||
| let _ = result_store | ||
| .complete(&record.scope, record.process_id, result.output) | ||
| .await; | ||
| } | ||
| } | ||
| Err(error) => { | ||
| if let Ok(record) = store.fail(&scope, process_id, error.kind).await | ||
| && let Some(result_store) = &result_store | ||
| && let Some(error_kind) = record.error_kind.clone() | ||
| { | ||
| let _ = result_store | ||
| .fail(&record.scope, record.process_id, error_kind) | ||
| .await; | ||
| } | ||
| } | ||
| } | ||
| if let Some(registry) = cancellation_registry { | ||
| registry.unregister(&scope, process_id); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The error handling within the spawned task for a background process is currently 'best effort' and silently ignores failures from store.complete and store.fail. These errors should be surfaced via tracing::warn! to ensure reliability and visibility. Additionally, if these functions handle errors internally and return (), they should not be awaited with the ? operator.
References
- When performing background persistence or state updates, errors should be surfaced via tracing::warn! to ensure reliability in short-lived or detached contexts.
- Functions that handle errors internally and return
()should not be awaited with the?operator.
| fn is_not_found(error: &FilesystemError) -> bool { | ||
| match error { | ||
| FilesystemError::Backend { reason, .. } => { | ||
| reason.contains("No such file") | ||
| || reason.contains("not found") | ||
| || reason.contains("entity not found") | ||
| } | ||
| _ => false, | ||
| } | ||
| } |
There was a problem hiding this comment.
This function relies on string matching on the error reason to detect 'not found' errors, which is brittle. To improve robustness and reduce duplication across crates, please introduce a specific NotFound variant to the FilesystemError enum in the ironclaw_filesystem crate. This allows for type-safe error handling and avoids relying on generic substring matching.
References
- Create specific error variants for different failure modes to provide semantically correct and clear error messages.
- When classifying errors by matching substrings, avoid overly generic patterns that can cause false positives. Prefer specific markers or explicit error variants.
| fn is_not_found(error: &FilesystemError) -> bool { | ||
| match error { | ||
| FilesystemError::Backend { reason, .. } => { | ||
| reason.contains("No such file") | ||
| || reason.contains("not found") | ||
| || reason.contains("os error 2") | ||
| } | ||
| _ => false, | ||
| } | ||
| } |
There was a problem hiding this comment.
This is_not_found helper function relies on brittle string matching. To improve robustness and reduce duplication, consider adding a specific NotFound variant to the FilesystemError enum in the ironclaw_filesystem crate. This would allow for type-safe error checking and remove the need for this helper function in multiple places.
References
- Create specific error variants for different failure modes to provide semantically correct and clear error messages.
- When classifying errors by matching substrings, avoid overly generic patterns that can cause false positives.
| fn is_not_found(error: &FilesystemError) -> bool { | ||
| match error { | ||
| FilesystemError::Backend { reason, .. } => { | ||
| reason.contains("No such file") | ||
| || reason.contains("not found") | ||
| || reason.contains("os error 2") | ||
| } | ||
| _ => false, | ||
| } | ||
| } |
There was a problem hiding this comment.
This is_not_found helper function is duplicated across several crates and relies on brittle string matching. Following repository standards, please introduce a specific NotFound variant to the FilesystemError enum in the ironclaw_filesystem crate to enable type-safe error handling and eliminate the need for substring matching.
References
- Create specific error variants for different failure modes to provide semantically correct and clear error messages.
- When classifying errors by matching substrings, avoid overly generic patterns that can cause false positives.
52a047d to
8b04d38
Compare
8b04d38 to
18e2857
Compare
18e2857 to
d81d27a
Compare
1 participant
Summary
Carves the smaller non-WASM runtime lane substrates from the Reborn stack.
Adds:
ironclaw_scriptsincludes:ScriptRuntimeScriptRuntimeConfigScriptExecutionRequest/ScriptExecutionResultScriptBackendcontract and backend request/output shapesironclaw_mcpincludes:McpRuntimeMcpRuntimeConfigMcpExecutionRequest/McpExecutionResultMcpClientcontract and client request/output shapesCurrent status
Rebased onto current
reborn-integrationafter the prerequisite substrate stack landed, including #3023, #3072, and #3076.The diff is now narrowed to only:
This slice is independent of the remaining in-flight Reborn PRs (#3028 WASM runtime lane and #3071 CapabilityHost base).
Scope boundary
This PR intentionally does not include:
ironclaw_wasm) — separate PRCapabilityHostExposure checklist
TDD note
Copied the Script/MCP contract tests first against RED stubs and confirmed
cargo test -p ironclaw_scripts -p ironclaw_mcpfailed due missing runtime/client/backend types before porting the implementations.Verification
Passed after rebasing onto current
reborn-integration:Boundary greps returned no forbidden normal Reborn dependencies.
Refs #2987.