feat(reborn): /v1/models, model validation, external-tool gate foundation - #5094
Merged
ilblackdragon merged 10 commits intoJun 26, 2026
Merged
Conversation
…tion
OpenAI-compatible Reborn surface work, in three parts:
1. /v1/models endpoint (#capability parity with pre-reborn):
- models DTO + OpenAiCompatModelCatalog host port + GET /v1/models
and /api/v1/models descriptors/handler/router state (fails closed
401/501 until wired); composition catalog backed by the operator
LlmConfigService snapshot; shared build_llm_config_service helper.
- Contract tests (descriptor lock 7->9, stub, models_handlers_contract)
+ unit tests for the list envelope and snapshot mapping.
2. model-name validation on chat + responses request parsing
(non-empty, no surrounding whitespace, no control chars, <=256 bytes
per the #2673 bounded-resources rule); unit + caller-level tests.
3. External-tool gate foundation (client-supplied tools on the Responses
API, staged epic):
- turns: BlockedExternalTool status family (status/reason/gate-kind/
precondition/pending-gate projection) + wire-stable round-trip tests.
- agent_loop executor pause: CapabilityOutcome::ExternalToolPending +
GateKind/LoopGateKind::ExternalTool + LoopBlockedKind::ExternalTool,
mapped through GateStage to a parked BlockedExternalTool turn.
- ExternalToolCatalog (turns): in-memory run-scoped catalog of caller
tool specs + input_ref<->call_id binding + submitted outputs; tests.
- ExternalToolCapabilityPort decorator (composition) wired into the
per-run capability chain via a factory-owned shared catalog: offers
caller tools to the model, rejects names shadowing host capabilities,
parks via ExternalToolPending, completes from the catalog on resume.
No production behavior change: the external-tool catalog is not yet fed
tool specs (Responses-side registration + executor resume re-dispatch are
follow-up stages), so the decorator is a safe no-op today.
Tests: turns, agent_loop, and reborn_openai_compat suites pass; composition
lib+tests+clippy clean; workspace compiles green.
|
🚅 Deployed to the ironclaw-pr-5094 environment in ironclaw-ci-preview
|
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5094
June 19, 2026 18:25
Destroyed
github-actions
Bot
added
scope: docs
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds external-tool gating and resume plumbing across the loop executor, OpenAI-compatible ChangesExternal Tool Gate
OpenAI-compat GET /v1/models Endpoint
Worker Stack Fix
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces support for client-supplied ("external") tools in the agent loop, allowing runs to park as BlockedExternalTool when an external tool is called and resume once the client submits the output. It adds an ExternalToolCatalog to manage these transient tool definitions and outputs, integrates them into the capability port wiring, and implements a GET /v1/models endpoint to list configured models. Additionally, validation is added for the client-supplied model field to enforce a 256-byte limit and reject malformed names. A high-severity issue was identified in the external tool capability port where the conflict check for duplicate sanitized capability IDs is broken, which could allow different tool names that sanitize to the same ID to silently overwrite each other.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+344
to
+356
| if surface | ||
| .descriptors | ||
| .iter() | ||
| .any(|descriptor| descriptor.capability_id == capability_id) | ||
| || capability_ids_by_tool_name | ||
| .insert(spec.name().to_string(), capability_id.clone()) | ||
| .is_some() | ||
| { | ||
| return Err(AgentLoopHostError::new( | ||
| AgentLoopHostErrorKind::InvalidInvocation, | ||
| "external tool conflicts with another capability id", | ||
| )); | ||
| } |
Contributor
There was a problem hiding this comment.
The conflict check for duplicate capability IDs is currently broken. It checks if capability_ids_by_tool_name.insert(spec.name().to_string(), ...) returns Some, but since spec.name() is unique (enforced by the catalog registration), this will always return None. As a result, different tool names that sanitize to the same capability_id (e.g., tool.a and tool_a both sanitizing to external_tool.tool_a) will silently overwrite each other in specs_by_capability_id without triggering a conflict error.
Instead, we should check if the sanitized capability_id already exists in specs_by_capability_id before inserting.
if surface
.descriptors
.iter()
.any(|descriptor| descriptor.capability_id == capability_id)
|| specs_by_capability_id.contains_key(&capability_id)
{
return Err(AgentLoopHostError::new(
AgentLoopHostErrorKind::InvalidInvocation,
"external tool conflicts with another capability id",
));
}
capability_ids_by_tool_name.insert(spec.name().to_string(), capability_id.clone());
Member
Author
There was a problem hiding this comment.
Fixed in commit d16d19a. Changed to check specs_by_capability_id.contains_key(&capability_id) instead of the broken capability_ids_by_tool_name.insert().is_some() which always returned None since tool names are unique by catalog registration.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_agent_loop/src/strategies/gate.rs (1)
62-68:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExternal-tool gates are currently skippable, which breaks park-and-resume semantics
GateOutcome::SkipAndContinueshould be invalid forGateKind::ExternalTool; otherwise the loop can proceed without client-submitted tool output.Suggested fix
pub(crate) fn validate_for_gate_kind(&self, kind: GateKind) -> Result<(), LoopFailureKind> { match (kind, self) { (GateKind::Approval, GateOutcome::SkipAndContinue { .. }) - | (GateKind::AwaitDependentRun, GateOutcome::SkipAndContinue { .. }) => { + | (GateKind::AwaitDependentRun, GateOutcome::SkipAndContinue { .. }) + | (GateKind::ExternalTool, GateOutcome::SkipAndContinue { .. }) => { Err(LoopFailureKind::DriverBug) } _ => Ok(()), } }for (variant, wire) in [ (GateKind::Approval, "approval"), (GateKind::Auth, "auth"), (GateKind::Resource, "resource"), (GateKind::AwaitDependentRun, "await_dependent_run"), + (GateKind::ExternalTool, "external_tool"), ] {Also applies to: 98-105
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_agent_loop/src/strategies/gate.rs` around lines 62 - 68, The GateKind::ExternalTool variant should not allow GateOutcome::SkipAndContinue as a valid outcome, since skipping external tool gates breaks park-and-resume semantics and allows the loop to proceed without client-submitted tool output. In the validation logic around lines 98-105 (likely a match statement handling different gate kinds), add a check that explicitly rejects or prevents SkipAndContinue outcomes for ExternalTool gates, ensuring that external tool gates require explicit completion or handling before the loop can continue.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_agent_loop/src/executor/capabilities.rs`:
- Around line 655-673: In the `ExternalToolPending` branch, verify that
`GateStage::process` properly clears the `state.pending_approval_resume` and
`state.pending_auth_resume` fields when handling `GateKind::ExternalTool`. If
`GateStage::process` does not normalize this resume state for external tool
gates, explicitly clear both `state.pending_approval_resume` and
`state.pending_auth_resume` before calling `GateStage.process()` in the
`ExternalToolPending` match arm to ensure stale approval/auth resume state is
not leaked.
In `@crates/ironclaw_reborn_composition/src/runtime.rs`:
- Around line 1821-1825: The code at lines 1821-1825 adds BlockedExternalTool to
the gate conditions that cause early return, but the docstring and nearby
comments for the containing function still describe only auth/approval/resource
gates. Update the function's docstring and any adjacent comments that document
gate-wait behavior to include BlockedExternalTool as one of the gate types that
causes short-circuiting and early return of the parked state, ensuring the
documentation accurately reflects the new behavior added to the TurnStatus
pattern match.
In `@crates/ironclaw_reborn_openai_compat/src/responses_workflow.rs`:
- Around line 1004-1006: The serde_json::from_slice call in the
OpenAiResponsesCreateRequest parsing is discarding the actual deserialization
error by using a wildcard pattern in map_err. Instead of ignoring the error with
|_|, capture it by binding it to a variable, log the error context using debug!
to preserve the cause for debugging, and then return the sanitized
invalid_request error. This preserves error context on the boundary parse path
while still returning a clean error response to the client.
In `@crates/ironclaw_reborn_openai_compat/tests/models_handlers_contract.rs`:
- Around line 66-97: The fail-closed parity checks in
models_endpoint_without_caller_returns_401_before_catalog and
models_endpoint_without_catalog_fails_closed_501 tests only assert behavior for
the /v1/models endpoint, but not the /api/v1/models alias endpoint. Add
equivalent assertions in both tests to verify the same 401 and 501 responses
respectively for the /api/v1/models path by making additional oneshot requests
to get_request("/api/v1/models") and asserting the same status codes and
response bodies to ensure the two endpoint paths maintain consistent fail-closed
behavior.
In
`@crates/ironclaw_reborn_openai_compat/tests/responses_workflow_handlers_contract.rs`:
- Around line 793-835: The test function
responses_rejects_invalid_model_before_product_workflow currently only validates
invalid model rejection for the /api/v1/responses endpoint. Add equivalent test
coverage for the /v1/responses endpoint to ensure both create routes enforce the
same validation contract. You can either iterate through both endpoint paths
within the test or add a duplicate test for the alternate endpoint path,
ensuring both routes properly reject oversized models, control characters, and
surrounding whitespace with a 400 BAD_REQUEST status and the expected error
structure before reaching the product workflow.
---
Outside diff comments:
In `@crates/ironclaw_agent_loop/src/strategies/gate.rs`:
- Around line 62-68: The GateKind::ExternalTool variant should not allow
GateOutcome::SkipAndContinue as a valid outcome, since skipping external tool
gates breaks park-and-resume semantics and allows the loop to proceed without
client-submitted tool output. In the validation logic around lines 98-105
(likely a match statement handling different gate kinds), add a check that
explicitly rejects or prevents SkipAndContinue outcomes for ExternalTool gates,
ensuring that external tool gates require explicit completion or handling before
the loop can continue.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 05da5d1f-3521-4805-b459-1f9ca7c8488f
📒 Files selected for processing (43)
crates/ironclaw_agent_loop/src/executor/capabilities.rscrates/ironclaw_agent_loop/src/executor/capability_helpers.rscrates/ironclaw_agent_loop/src/executor/mapping.rscrates/ironclaw_agent_loop/src/strategies/gate.rscrates/ironclaw_event_projections/src/pending_gate_projection.rscrates/ironclaw_loop_support/src/turn_event_publisher.rscrates/ironclaw_reborn/src/subagent/completion_observer.rscrates/ironclaw_reborn_composition/src/openai_compat_serve.rscrates/ironclaw_reborn_composition/src/openai_compat_serve/tests.rscrates/ironclaw_reborn_composition/src/projection/turn_events.rscrates/ironclaw_reborn_composition/src/runtime.rscrates/ironclaw_reborn_composition/src/runtime/local_dev.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/external_tool_capability.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/refreshing_capability_port.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/shell_tests.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/tests.rscrates/ironclaw_reborn_composition/src/trigger_poller.rscrates/ironclaw_reborn_composition/src/webui.rscrates/ironclaw_reborn_openai_compat/CLAUDE.mdcrates/ironclaw_reborn_openai_compat/src/chat_workflow.rscrates/ironclaw_reborn_openai_compat/src/descriptors.rscrates/ironclaw_reborn_openai_compat/src/handlers.rscrates/ironclaw_reborn_openai_compat/src/lib.rscrates/ironclaw_reborn_openai_compat/src/model_validation.rscrates/ironclaw_reborn_openai_compat/src/models.rscrates/ironclaw_reborn_openai_compat/src/models_catalog.rscrates/ironclaw_reborn_openai_compat/src/responses_workflow.rscrates/ironclaw_reborn_openai_compat/src/router.rscrates/ironclaw_reborn_openai_compat/tests/chat_workflow_handlers_contract.rscrates/ironclaw_reborn_openai_compat/tests/descriptors_contract.rscrates/ironclaw_reborn_openai_compat/tests/models_handlers_contract.rscrates/ironclaw_reborn_openai_compat/tests/responses_workflow_handlers_contract.rscrates/ironclaw_reborn_openai_compat/tests/stub_handlers_contract.rscrates/ironclaw_turns/src/events.rscrates/ironclaw_turns/src/external_tool_catalog.rscrates/ironclaw_turns/src/lib.rscrates/ironclaw_turns/src/lifecycle.rscrates/ironclaw_turns/src/loop_exit.rscrates/ironclaw_turns/src/loop_exit/tests/mod.rscrates/ironclaw_turns/src/request.rscrates/ironclaw_turns/src/run_profile/host.rscrates/ironclaw_turns/src/status.rscrates/ironclaw_turns/tests/turn_coordinator_contract.rs
Completes the engine half of the client-tool round-trip: a parked BlockedExternalTool run now re-dispatches the client tool on resume and completes from the run-scoped catalog output, without a model turn. - agent_loop state: new pending_external_tool_resume slot + struct. - GateStage populates the slot at block time for GateKind::ExternalTool. - prompt/canonical: PromptStep::ResumeExternalTool re-dispatches the parked call (re-registering the provider tool call so the host decorator re-binds input_ref->call_id and restages arguments). - capability_helpers: pending_external_tool_resume_candidate + clear_matching_pending_external_tool_resume (cleared at every outcome site, mirroring the auth/approval slots). - capabilities: denied-guard so a cancelled external-tool gate surfaces a model-visible failure instead of re-parking forever. - planned_driver: stamp_resume_disposition extended to the external-tool slot. Tests: two executor integration tests (block stores the slot; resume re-dispatches without a model turn and completes); 323 agent_loop tests pass; workspace green; clippy clean.
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5094
June 19, 2026 19:09
Destroyed
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_agent_loop/src/executor/capability_helpers.rs`:
- Around line 504-538: The validation in the
pending_external_tool_resume_candidate function only checks that
candidate.capability_id matches resume.capability_id, but is missing the
effective_capability_ids consistency check that exists in the similar
pending_auth_resume_candidate function. Update the if statement that validates
the candidate to also check that candidate.effective_capability_ids equals
resume.effective_capability_ids by adding an OR condition, matching the dual
validation pattern from pending_auth_resume_candidate to ensure consistency and
prevent drift.
In `@crates/ironclaw_reborn/src/planned_driver.rs`:
- Around line 270-299: The stamp_resume_disposition function now handles the
external_tool_matches case for stamping pending_external_tool_resume, but this
path lacks corresponding unit test coverage. Add a new unit test (following the
naming pattern of existing tests like
stamp_resume_disposition_stamps_auth_slot_when_last_gate_matches) that verifies
the external tool resume slot gets stamped correctly when
pending_external_tool_resume exists with a matching gate_ref. This test should
parallel the existing auth-only and approval-only test cases to ensure
consistent coverage of all three resume paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ae913f87-da19-4b09-b5a8-7d9a93fbe2ec
📒 Files selected for processing (9)
crates/ironclaw_agent_loop/src/executor.rscrates/ironclaw_agent_loop/src/executor/canonical.rscrates/ironclaw_agent_loop/src/executor/capabilities.rscrates/ironclaw_agent_loop/src/executor/capability_helpers.rscrates/ironclaw_agent_loop/src/executor/gates.rscrates/ironclaw_agent_loop/src/executor/prompt.rscrates/ironclaw_agent_loop/src/executor/tests.rscrates/ironclaw_agent_loop/src/state.rscrates/ironclaw_reborn/src/planned_driver.rs
…se 4a) Move the Arc<dyn ExternalToolCatalog> onto RebornLocalRuntimeServices so the loop capability host (which offers caller tools and parks calls) and the OpenAI-compatible Responses surface (which will register tool specs and submit client outputs) share one run-scoped instance. The loop factory now reads the shared catalog instead of constructing its own. No behavior change yet — the catalog is still not fed specs until the Responses-side wiring lands.
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5094
June 19, 2026 19:35
Destroyed
…mpat-models-and-external-tools Resolve semantic conflict: main added a new project_create test that constructs LocalDevLoopCapabilityPortFactory, which this branch extended with a required external_tool_catalog field. Add the field to the new initializer (crates/ironclaw_reborn_composition/src/runtime/local_dev/tests.rs) to fix the E0063 compile error in the merged tree.
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5094
June 22, 2026 05:02
Destroyed
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_reborn_composition/src/trigger_poller.rs (1)
324-324:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd regression coverage for the new
BlockedExternalToolterminal mapping.Line 324 adds a new terminal-status branch, but the table test at Line 475 does not assert
TurnStatus::BlockedExternalTool -> TriggerRunHistoryStatus::Error. Please add that case so this mapping cannot regress silently.Suggested test delta
fn terminal_turn_statuses_map_to_run_history_statuses() { let cases = [ (TurnStatus::Completed, TriggerRunHistoryStatus::Ok), (TurnStatus::Cancelled, TriggerRunHistoryStatus::Error), (TurnStatus::Failed, TriggerRunHistoryStatus::Error), (TurnStatus::RecoveryRequired, TriggerRunHistoryStatus::Error), + (TurnStatus::BlockedExternalTool, TriggerRunHistoryStatus::Error), ];As per coding guidelines, “Every bug fix must include a regression test (
#[test]or#[tokio::test]).”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_reborn_composition/src/trigger_poller.rs` at line 324, The new TurnStatus::BlockedExternalTool terminal status mapping added at line 324 lacks a corresponding regression test case. Locate the table test around line 475 that tests the terminal-status-to-TriggerRunHistoryStatus mappings and add a test case that asserts TurnStatus::BlockedExternalTool maps to TriggerRunHistoryStatus::Error to prevent this mapping from silently regressing in the future.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/ironclaw_reborn_composition/src/trigger_poller.rs`:
- Line 324: The new TurnStatus::BlockedExternalTool terminal status mapping
added at line 324 lacks a corresponding regression test case. Locate the table
test around line 475 that tests the terminal-status-to-TriggerRunHistoryStatus
mappings and add a test case that asserts TurnStatus::BlockedExternalTool maps
to TriggerRunHistoryStatus::Error to prevent this mapping from silently
regressing in the future.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: eb5125d7-b077-4669-8298-a9a705b4bc1f
📒 Files selected for processing (7)
crates/ironclaw_reborn_composition/src/factory.rscrates/ironclaw_reborn_composition/src/runtime.rscrates/ironclaw_reborn_composition/src/runtime/local_dev.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/refreshing_capability_port.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/shell_tests.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/tests.rscrates/ironclaw_reborn_composition/src/trigger_poller.rs
…tack overflow The reborn QA smoke scenario `qa_error_process_repo_patch_and_cleanup_smokes` aborted with `fatal runtime error: stack overflow`. A single capability-dispatch poll of the agent loop (turn runner -> planned driver -> canonical executor -> capability stage -> host dispatch -> first-party tool) consumes ~1.9 MB of stack in debug builds, overflowing the default 2 MB worker thread. The chain was already borderline on main; this branch's external-tool plumbing tipped it over. Boxing the hot futures does not help here: in debug builds `Box::pin(fut)` still constructs the whole future on the stack before moving it to the heap, so the poll frame's peak is unchanged (verified by measurement). The codebase already handles deep work by enlarging the thread stack (see the ironclaw_reborn_cli traces tests and the src/cli stack_size sites), so do the same: - serve runtime: set thread_stack_size(8 MB) on the production multi-thread tokio runtime so the turn-runner worker poll runs with adequate headroom. - QA harness: run each turn-runner worker on a dedicated 8 MB std::thread with its own current-thread runtime, because `#[tokio::test]` runs spawned tasks on the libtest 2 MB thread and exposes no thread_stack_size knob. Regression coverage: qa_error_process_repo_patch_and_cleanup_smokes and the other harness-based QA/parity suites now pass at the default test stack.
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5094
June 22, 2026 08:28
Destroyed
- Fix broken duplicate capability_id conflict check in external_tool_capability.rs: check specs_by_capability_id instead of capability_ids_by_tool_name.insert().is_some() which always returns None since tool names are unique by catalog registration - Add GateKind::ExternalTool to SkipAndContinue rejection in validate_for_gate_kind and round-trip/default-handler tests - Add effective_capability_ids consistency check to pending_external_tool_resume_candidate matching the dual-validation pattern from pending_auth_resume_candidate - Preserve serde deserialization error context in parse_response_create_request with tracing::debug before returning sanitized error - Update wait_for_terminal_or_gate docstring and comments to include external-tool as a client-resolvable gate - Add /api/v1/models alias assertions to 401/501 fail-closed tests - Add /v1/responses path to invalid-model rejection test - Add stamp_resume_disposition unit test for external-tool resume slot
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5094
June 25, 2026 22:31
Destroyed
- Added activity_id field to PendingExternalToolResume (main added CapabilityActivityId to CapabilityCallCandidate / RegisterProviderToolCallRequest) - Fixed short_circuit_denied_resume call to pass CapabilityActivityId instead of CapabilityId, removed extra None arg - Added TurnBlockedGateKind::ExternalTool arms in turn_events.rs - Fixed register_provider_tool_call impl in external_tool_capability.rs to accept RegisterProviderToolCallRequest - Added activity_id to CapabilityCallCandidate construction - Removed unused capability_activity_id_from_resume_token helper - Accepted main's trigger_poller submodule extraction, scheduler refactor in harness.rs, auto-approve/permissions crate split
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5094
June 25, 2026 23:19
Destroyed
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_agent_loop/src/executor/capability_helpers.rs (1)
519-556: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep external-tool resumes on the parked activity id.
This helper still remints activity identity: provider-backed resumes call
RegisterProviderToolCallRequest::new(...), and the staged-input fallback usesCapabilityActivityId::new(). The auth path already preservesresume.activity_id_for_resume(). With the current code, an external-tool resume can complete/fail under a different activity than the one parked inGateStage, which breaks the activity-scoped denied/completion flow inCapabilityStage.Suggested fix
pub(super) async fn pending_external_tool_resume_candidate( host: &(dyn AgentLoopDriverHost + Send + Sync), resume: &PendingExternalToolResume, surface_version: CapabilitySurfaceVersion, ) -> Result<CapabilityCallCandidate, AgentLoopExecutorError> { if let Some(replay) = resume.provider_replay.as_ref() { let candidate = host - .register_provider_tool_call(RegisterProviderToolCallRequest::new(ProviderToolCall { - provider_id: replay.provider_id.clone(), - provider_model_id: replay.provider_model_id.clone(), - turn_id: Some(replay.provider_turn_id.clone()), - id: replay.provider_call_id.clone(), - name: replay.provider_tool_name.clone(), - arguments: replay.arguments.clone(), - response_reasoning: replay.response_reasoning.clone(), - reasoning: replay.reasoning.clone(), - signature: replay.signature.clone(), - })) + .register_provider_tool_call(RegisterProviderToolCallRequest::for_activity( + ProviderToolCall { + provider_id: replay.provider_id.clone(), + provider_model_id: replay.provider_model_id.clone(), + turn_id: Some(replay.provider_turn_id.clone()), + id: replay.provider_call_id.clone(), + name: replay.provider_tool_name.clone(), + arguments: replay.arguments.clone(), + response_reasoning: replay.response_reasoning.clone(), + reasoning: replay.reasoning.clone(), + signature: replay.signature.clone(), + }, + resume.activity_id_for_resume(), + )) .await .map_err(capability_host_error)?; - if candidate.capability_id != resume.capability_id + if candidate.activity_id != resume.activity_id_for_resume() + || candidate.capability_id != resume.capability_id || candidate.effective_capability_ids != resume.effective_capability_ids { return Err(AgentLoopExecutorError::PlannerContract { detail: "external tool resume provider replay no longer matches blocked capability", }); } return Ok(candidate); } Ok(CapabilityCallCandidate { - activity_id: ironclaw_turns::CapabilityActivityId::new(), + activity_id: resume.activity_id_for_resume(), surface_version, capability_id: resume.capability_id.clone(), input_ref: resume.input_ref.clone(), effective_capability_ids: resume.effective_capability_ids.clone(), provider_replay: resume.provider_replay.clone(),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_agent_loop/src/executor/capability_helpers.rs` around lines 519 - 556, The external-tool resume flow is still minting a fresh activity instead of reusing the parked one. Update pending_external_tool_resume_candidate so both the provider_replay path and the staged-input fallback preserve resume.activity_id_for_resume() rather than relying on RegisterProviderToolCallRequest::new or CapabilityActivityId::new(). Make sure the returned CapabilityCallCandidate carries the parked activity id consistently with the auth path so CapabilityStage stays activity-scoped.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_agent_loop/src/state.rs`:
- Around line 32-37: The checkpoint schema change in state.rs removes support
for existing reborn:default-loop-v1 payloads, which will block resume for
in-flight runs. Update the checkpoint handling around CHECKPOINT_SCHEMA_ID and
CHECKPOINT_SCHEMA_VERSION to remain backward-compatible by accepting v1 during
decode, or add an explicit migration path before enforcing the new v2 schema so
older checkpoints can still be resumed.
---
Outside diff comments:
In `@crates/ironclaw_agent_loop/src/executor/capability_helpers.rs`:
- Around line 519-556: The external-tool resume flow is still minting a fresh
activity instead of reusing the parked one. Update
pending_external_tool_resume_candidate so both the provider_replay path and the
staged-input fallback preserve resume.activity_id_for_resume() rather than
relying on RegisterProviderToolCallRequest::new or CapabilityActivityId::new().
Make sure the returned CapabilityCallCandidate carries the parked activity id
consistently with the auth path so CapabilityStage stays activity-scoped.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e773e2cf-0372-4981-883f-6ccca8d87020
📒 Files selected for processing (10)
crates/ironclaw_agent_loop/src/executor/capabilities.rscrates/ironclaw_agent_loop/src/executor/capability_helpers.rscrates/ironclaw_agent_loop/src/executor/gates.rscrates/ironclaw_agent_loop/src/executor/mapping.rscrates/ironclaw_agent_loop/src/executor/tests.rscrates/ironclaw_agent_loop/src/state.rscrates/ironclaw_loop_support/src/turn_event_publisher.rscrates/ironclaw_reborn/src/planned_driver.rscrates/ironclaw_reborn/src/subagent/completion_observer.rscrates/ironclaw_reborn_cli/src/commands/serve.rs
💤 Files with no reviewable changes (4)
- crates/ironclaw_loop_support/src/turn_event_publisher.rs
- crates/ironclaw_reborn/src/subagent/completion_observer.rs
- crates/ironclaw_reborn_cli/src/commands/serve.rs
- crates/ironclaw_reborn/src/planned_driver.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_agent_loop/src/executor/capability_helpers.rs (1)
519-556: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep external-tool resumes on the parked activity id.
This helper still remints activity identity: provider-backed resumes call
RegisterProviderToolCallRequest::new(...), and the staged-input fallback usesCapabilityActivityId::new(). The auth path already preservesresume.activity_id_for_resume(). With the current code, an external-tool resume can complete/fail under a different activity than the one parked inGateStage, which breaks the activity-scoped denied/completion flow inCapabilityStage.Suggested fix
pub(super) async fn pending_external_tool_resume_candidate( host: &(dyn AgentLoopDriverHost + Send + Sync), resume: &PendingExternalToolResume, surface_version: CapabilitySurfaceVersion, ) -> Result<CapabilityCallCandidate, AgentLoopExecutorError> { if let Some(replay) = resume.provider_replay.as_ref() { let candidate = host - .register_provider_tool_call(RegisterProviderToolCallRequest::new(ProviderToolCall { - provider_id: replay.provider_id.clone(), - provider_model_id: replay.provider_model_id.clone(), - turn_id: Some(replay.provider_turn_id.clone()), - id: replay.provider_call_id.clone(), - name: replay.provider_tool_name.clone(), - arguments: replay.arguments.clone(), - response_reasoning: replay.response_reasoning.clone(), - reasoning: replay.reasoning.clone(), - signature: replay.signature.clone(), - })) + .register_provider_tool_call(RegisterProviderToolCallRequest::for_activity( + ProviderToolCall { + provider_id: replay.provider_id.clone(), + provider_model_id: replay.provider_model_id.clone(), + turn_id: Some(replay.provider_turn_id.clone()), + id: replay.provider_call_id.clone(), + name: replay.provider_tool_name.clone(), + arguments: replay.arguments.clone(), + response_reasoning: replay.response_reasoning.clone(), + reasoning: replay.reasoning.clone(), + signature: replay.signature.clone(), + }, + resume.activity_id_for_resume(), + )) .await .map_err(capability_host_error)?; - if candidate.capability_id != resume.capability_id + if candidate.activity_id != resume.activity_id_for_resume() + || candidate.capability_id != resume.capability_id || candidate.effective_capability_ids != resume.effective_capability_ids { return Err(AgentLoopExecutorError::PlannerContract { detail: "external tool resume provider replay no longer matches blocked capability", }); } return Ok(candidate); } Ok(CapabilityCallCandidate { - activity_id: ironclaw_turns::CapabilityActivityId::new(), + activity_id: resume.activity_id_for_resume(), surface_version, capability_id: resume.capability_id.clone(), input_ref: resume.input_ref.clone(), effective_capability_ids: resume.effective_capability_ids.clone(), provider_replay: resume.provider_replay.clone(),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_agent_loop/src/executor/capability_helpers.rs` around lines 519 - 556, The external-tool resume flow is still minting a fresh activity instead of reusing the parked one. Update pending_external_tool_resume_candidate so both the provider_replay path and the staged-input fallback preserve resume.activity_id_for_resume() rather than relying on RegisterProviderToolCallRequest::new or CapabilityActivityId::new(). Make sure the returned CapabilityCallCandidate carries the parked activity id consistently with the auth path so CapabilityStage stays activity-scoped.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_agent_loop/src/state.rs`:
- Around line 32-37: The checkpoint schema change in state.rs removes support
for existing reborn:default-loop-v1 payloads, which will block resume for
in-flight runs. Update the checkpoint handling around CHECKPOINT_SCHEMA_ID and
CHECKPOINT_SCHEMA_VERSION to remain backward-compatible by accepting v1 during
decode, or add an explicit migration path before enforcing the new v2 schema so
older checkpoints can still be resumed.
---
Outside diff comments:
In `@crates/ironclaw_agent_loop/src/executor/capability_helpers.rs`:
- Around line 519-556: The external-tool resume flow is still minting a fresh
activity instead of reusing the parked one. Update
pending_external_tool_resume_candidate so both the provider_replay path and the
staged-input fallback preserve resume.activity_id_for_resume() rather than
relying on RegisterProviderToolCallRequest::new or CapabilityActivityId::new().
Make sure the returned CapabilityCallCandidate carries the parked activity id
consistently with the auth path so CapabilityStage stays activity-scoped.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e773e2cf-0372-4981-883f-6ccca8d87020
📒 Files selected for processing (10)
crates/ironclaw_agent_loop/src/executor/capabilities.rscrates/ironclaw_agent_loop/src/executor/capability_helpers.rscrates/ironclaw_agent_loop/src/executor/gates.rscrates/ironclaw_agent_loop/src/executor/mapping.rscrates/ironclaw_agent_loop/src/executor/tests.rscrates/ironclaw_agent_loop/src/state.rscrates/ironclaw_loop_support/src/turn_event_publisher.rscrates/ironclaw_reborn/src/planned_driver.rscrates/ironclaw_reborn/src/subagent/completion_observer.rscrates/ironclaw_reborn_cli/src/commands/serve.rs
💤 Files with no reviewable changes (4)
- crates/ironclaw_loop_support/src/turn_event_publisher.rs
- crates/ironclaw_reborn/src/subagent/completion_observer.rs
- crates/ironclaw_reborn_cli/src/commands/serve.rs
- crates/ironclaw_reborn/src/planned_driver.rs
🛑 Comments failed to post (1)
crates/ironclaw_agent_loop/src/state.rs (1)
32-37: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Preserve v1 checkpoint readability during rollout.
Lines 34-35 explicitly drop v1 support. That makes any run blocked on a
reborn:default-loop-v1checkpoint unresumable after deploy, because resume validates the stored schema id/version before loading the payload. Keep the decoder backward-compatible or add an explicit migration path before switching the schema id.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_agent_loop/src/state.rs` around lines 32 - 37, The checkpoint schema change in state.rs removes support for existing reborn:default-loop-v1 payloads, which will block resume for in-flight runs. Update the checkpoint handling around CHECKPOINT_SCHEMA_ID and CHECKPOINT_SCHEMA_VERSION to remain backward-compatible by accepting v1 during decode, or add an explicit migration path before enforcing the new v2 schema so older checkpoints can still be resumed.
Use RegisterProviderToolCallRequest::for_activity() with resume.activity_id_for_resume() (matching the auth-resume path) instead of ::new() which allocates a fresh activity id. Also validate candidate.activity_id in the replay-match check, and use resume.activity_id_for_resume() in the staged-input fallback instead of CapabilityActivityId::new().
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5094
June 26, 2026 00:16
Destroyed
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5094
June 26, 2026 05:39
Destroyed
ilblackdragon
deleted the
feat/reborn-openai-compat-models-and-external-tools
branch
June 26, 2026 06:05
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
OpenAI-compatible Reborn surface work in three parts. No production behavior change — the external-tool catalog is not yet fed tool specs (Responses-side registration + executor resume re-dispatch are follow-up stages), so the new decorator is a safe no-op today.
1.
/v1/models(capability parity with pre-reborn)modelsDTO +OpenAiCompatModelCataloghost port +GET /v1/modelsand/api/v1/modelsdescriptors / handler / router state. Fails closed (401 without auth, 501 without a wired catalog) — same pattern as chat/responses.LlmConfigServicesnapshot (deduped active + provider models), gated onroot-llm-provider; CLI already mounts it. Extracted a sharedbuild_llm_config_servicehelper so WebUI and openai-compat read one source.models_handlers_contract, list-envelope + snapshot-mapping unit tests.2. Model-name validation
validate_model_nameon both chat and responses request parsing (non-empty, no surrounding whitespace, no control chars, ≤256 bytes per the feat(llm): hot-reload provider chain from settings (supersedes #2059) #2673 bounded-resources rule). Unit + caller-level contract tests (chat + responses).3. External-tool gate foundation (staged epic)
Client-supplied tools on the Responses API, where the agent loop pauses and hands control back to the API client.
BlockedExternalToolstatus family (status / reason / gate-kind / precondition / pending-gate projection) + wire-stable round-trip tests.CapabilityOutcome::ExternalToolPending+GateKind/LoopGateKind::ExternalTool+LoopBlockedKind::ExternalTool, mapped throughGateStageto a parkedBlockedExternalToolturn.ExternalToolCatalog(turns): in-memory, run-scoped catalog of caller tool specs +input_ref↔call_idbinding + submitted outputs; 8 tests.ExternalToolCapabilityPortdecorator (composition), wired into the per-run capability chain via a factory-owned shared catalog: offers caller tools to the model, rejects names shadowing host capabilities, parks viaExternalToolPending, and completes from the catalog on resume.Remaining (follow-up stages, this branch)
pending_external_tool_resumeslot, mirroringpending_auth_resume).ExternalToolInteractionService(submit output → catalog →resume_turn).tools→ register into catalog (via a new openai_compat port), parked call →function_callitem,function_call_output+previous_response_id→ resume; streaming + non-streaming; expose the catalog singleton tobuild_openai_compat_route_mount.tests/e2e_responses_api_external_tools.rs.Validation
cargo fmtclean; composition lib+tests+clippy clean; turns / agent_loop / reborn_openai_compat suites pass; workspace compiles green.