Skip to content

fix(reborn): harden capability approval lifecycle - #3111

Merged
serrrfirat merged 1 commit into
reborn-integrationfrom
reborn-fix-capability-host-review
Apr 30, 2026
Merged

serrrfirat merged 1 commit into
reborn-integrationfrom
reborn-fix-capability-host-review

Conversation

@serrrfirat

@serrrfirat serrrfirat commented Apr 30, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Follow-up to #3071 after the capability-host base PR landed. Hardens the approval/run-state/lease lifecycle around the review findings:

  • Persist spawn approval requests instead of failing terminally when authorize_spawn_with_trust returns RequireApproval.
  • Add CapabilityHost::resume_spawn_json so approved spawn requests can claim the matching lease, start the process, consume the lease, and complete run state.
  • Add spawn-specific invocation fingerprints so dispatch and spawn approvals cannot share the same replay fingerprint.
  • Validate approval correlation_id in addition to action/requester/fingerprint before persisting or resuming approvals.
  • Revoke a claimed approval lease when resumed dispatch fails after the claim but before successful runtime dispatch.
  • Make filesystem run/approval listings tolerate records deleted after directory listing, preventing rollback/listing races from failing the whole list.

Notes

  • Still no concrete runtime/dispatcher/WASM dependency from ironclaw_capabilities.
  • Existing dispatch approval path is preserved; approve_dispatch now delegates through a shared action-specific helper, and approve_spawn handles Action::SpawnCapability.
  • I self-reviewed this diff directly; no review subagent was used.
  • skip-regression-check label and [skip-regression-check] commit marker are applied because the workflow false-negatives on crate-local Rust test changes, even though this PR adds multiple regression tests.

Verification

cargo test -p ironclaw_host_api
cargo test -p ironclaw_approvals
cargo test -p ironclaw_run_state
cargo test -p ironclaw_capabilities
cargo clippy -p ironclaw_host_api -p ironclaw_approvals -p ironclaw_run_state -p ironclaw_capabilities --all-targets -- -D warnings
cargo test -p ironclaw_architecture
cargo fmt --all -- --check
git diff --check

All passed locally.

@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 Apr 30, 2026
Follow-up to the capability host base slice.\n\n[skip-regression-check]
@serrrfirat
serrrfirat force-pushed the reborn-fix-capability-host-review branch from 6c13dc5 to b523db2 Compare April 30, 2026 12:00
@serrrfirat serrrfirat added the skip-regression-check Bypass regression test CI gate (tests exist but not in tests/ dir) label Apr 30, 2026
@serrrfirat
serrrfirat merged commit 2d727be into reborn-integration Apr 30, 2026
16 checks passed
@serrrfirat
serrrfirat deleted the reborn-fix-capability-host-review branch April 30, 2026 12:02

@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 adds support for asynchronous process spawn approvals, aligning it with the existing dispatch approval flow. Key updates include refactoring the approval resolution logic, modifying the capability host to handle pending spawn requests and run-state transitions, and implementing lease revocation on invocation failure. Additionally, filesystem-based stores now ignore missing files during directory scans. Feedback recommends centralizing the duplicated file-reading logic within the run-state stores into a helper function to improve maintainability and consistency.

Comment on lines +664 to +668
let bytes = match self.filesystem.read_file(&entry.path).await {
Ok(bytes) => bytes,
Err(error) if is_not_found(&error) => continue,
Err(error) => return Err(error.into()),
};

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

This logic to gracefully handle NotFound errors is duplicated in FilesystemApprovalRequestStore::records_for_scope (lines 826-830). To improve maintainability and ensure consistent behavior, consider extracting this into a centralized helper function that handles errors gracefully by returning None, as per repository guidelines for resource parsing. Additionally, use the let-else pattern to skip missing files without aborting the scan.

Example helper:

async fn read_file_tolerant<F: RootFilesystem>(
    filesystem: &F,
    path: &VirtualPath,
) -> Result<Option<Vec<u8>>, RunStateError> {
    match filesystem.read_file(path).await {
        Ok(bytes) => Ok(Some(bytes)),
        Err(error) if is_not_found(&error) => Ok(None),
        Err(error) => Err(error.into()),
    }
}

Example usage:

let Some(bytes) = read_file_tolerant(self.filesystem, &entry.path).await? else {
    continue;
};
References
  1. Centralize parsing logic for resources that can fail into a single function that handles errors gracefully (e.g., by returning None) to ensure consistent behavior across all call sites.
  2. Use let Some(...) = ... else { continue; } instead of the ? operator when scanning resources to skip items that don't match the expected format without prematurely aborting the entire scan.

This was referenced May 7, 2026
@ironclaw-ci ironclaw-ci Bot mentioned this pull request May 8, 2026
theredspoon pushed a commit to theredspoon/ironclaw that referenced this pull request Jun 21, 2026
Follow-up to the capability host base slice.\n\n[skip-regression-check]
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 skip-regression-check Bypass regression test CI gate (tests exist but not in tests/ dir)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant