fix(engine): always append ActionResult for every tool call

[ilblackdragon]

Summary

Fixes HTTP 400 No tool output found for function call <id> from OpenAI's Responses API when a tool call in the assistant message has no corresponding tool result.

The Python orchestrator only appended ActionResult messages when output is not None, leaving dangling tool_calls in working_messages in three scenarios:

1. Tool returns null output - JSON null -> Python None, skipping the append
2. Gate-paused results - result_json has no output field at all
3. RequireApproval preflight early return - Rust returned fewer results than calls, so trailing calls got no slot

OpenAI's Responses API requires 1:1 correspondence between function_call and function_call_output items, so the next LLM turn fails with the 400.

Changes

• crates/ironclaw_engine/orchestrator/default.py - iterate over executable_calls (not results) and emit an ActionResult for every call, with "[no output]" / "[execution skipped]" placeholders when the real output is missing.
• crates/ironclaw_engine/src/executor/orchestrator.rs - pad the RequireApproval early-return results_json to parsed.len() so Python sees a null slot for every call and emits the placeholder.
• Regression tests at both layers:
  • json_to_thread_messages_every_tool_call_has_action_result (engine) - parse path
  • tool_call_result_correspondence_after_sanitize (bridge) - full ThreadMessage -> ChatMessage -> sanitize_tool_messages pipeline

Test plan

• cargo test -p ironclaw_engine (350 pass, including new regression test)
• cargo test --lib -- bridge::llm_adapter (new regression test passes)
• cargo clippy --lib --bins -- -D warnings clean
• Rerun the failing update all tools flow against the OpenAI Codex provider and confirm no more HTTP 400

🤖 Generated with Claude Code

[gemini-code-assist]

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

[Copilot]

Pull request overview

Fixes OpenAI Responses API HTTP 400s caused by missing tool output items by ensuring every assistant tool call has a corresponding tool result placeholder across the Rust executor and Python orchestrator, with regression tests to prevent recurrence.

Changes:

• Python orchestrator now appends an ActionResult for every emitted tool call (using placeholders when output is missing/skipped).
• Rust RequireApproval early-return path pads results_json so Python receives a slot per tool call.
• Adds regression tests in both engine parsing and bridge sanitization pipelines to enforce 1:1 tool-call/result correspondence.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
crates/ironclaw_engine/orchestrator/default.py	Ensures an ActionResult is appended for every tool call (placeholders for missing/skipped output).
crates/ironclaw_engine/src/executor/orchestrator.rs	Pads results_json with nulls on approval early return; adds parsing regression test.
src/bridge/llm_adapter.rs	Adds regression test to ensure sanitization preserves tool-call ↔ tool-message correspondence.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

This regression test can pass even if action_calls fail to parse (e.g., schema drift) because assistant_calls would be empty and the loop asserting correspondence would never run. Add an assertion that the assistant call set has the expected size (and/or is non-empty) so the test actually verifies tool-call extraction before checking ActionResult coverage.

Suggested change

	.collect();
	.collect();
	assert_eq!(
	assistant_calls.len(),
	3,
	"expected to extract all assistant action_calls from the fixture"
	);

[ilblackdragon]

Good catch — addressed in the follow-up commit by adding an explicit assert_eq!(assistant_calls.len(), 3, ...) right after the HashSet collection. The test will now fail loudly if schema drift causes action_calls to silently deserialize into None, instead of vacuously passing the correspondence loop.

Also extended coverage with a new fixture test json_to_thread_messages_handles_empty_and_malformed_action_results that drives the parser with each placeholder shape Python emits ([no output], [execution skipped], empty string, and a missing content field), plus a caller-level companion in src/bridge/llm_adapter.rs (complete_with_tools_preserves_empty_and_placeholder_action_results) that asserts the provider ultimately receives a Tool message for every tool_call even when the content is empty or a placeholder.

[serrrfirat]

Medium Severity — Edge case documentation

The bounds check results[idx] if idx < len(results) else None is correct, but the comment (line 724-727) only mentions one scenario: "calls that the Rust batch handler skipped (e.g. RequireApproval early return)".

There's a second scenario worth documenting: if a JoinSet task panics in the parallel execution path (orchestrator.rs:1483-1485), the slot gets a fallback {"is_error": true, "output": {"error": "execution slot empty"}} — so the result will be non-null with a synthetic error. The Python code handles this correctly (it emits str({...}) as output), but future maintainers may not realize this path exists.

Suggestion: Widen the comment to say something like "calls that the Rust batch handler skipped or couldn't reach (RequireApproval early return, task panic recovery, etc.)"

[serrrfirat]

Medium Severity — Missing direct test of Rust padding logic

This test validates that json_to_thread_messages correctly parses working_messages where every tool call has an ActionResult — but it doesn't exercise the actual Rust padding code (while results_json.len() < parsed.len() at line 1323).

The padding loop is the critical Rust-side fix. If someone removes it, this test still passes because it only tests the json_to_thread_messages parse function, not handle_execute_actions_parallel. The Python-side defense (idx < len(results)) would catch the gap at runtime, so this isn't a ship-blocker — but a direct unit test of the padding would strengthen the safety net.

Suggestion: Add a test that invokes handle_execute_actions_parallel with a mock PolicyEngine that returns RequireApproval on the 2nd of 3 calls, and asserts the returned JSON array has length 3 (matching input), not 2.

[serrrfirat]

Solid bug fix with good defense-in-depth across both Python and Rust layers. The dual-layer protection (Rust pads results + Python handles short lists) is well-engineered. Two medium-severity suggestions posted inline — neither is a ship-blocker. LGTM.