feat(llm): scaffold tool_args.rs shared parsing primitives (RC3/M9 Phase A)

[henrypark133]

Summary

• Phase A of the RC3/M9 provider parsing framework — installs crates/ironclaw_llm/src/tool_args.rs (Layer 2 shared utility primitives).
• No callers yet. Phase B adds ToolCall.arguments_parse_error; Phase C adds the NormalizingProvider decorator that closes audit RC1 universally.
• Mirrors hybrid architecture confirmed in prior-art investigations of pi and opencode (each shipping 10+ providers): per-provider wire decoders + shared utility primitives, not a single agnostic decorator.

What this PR adds

• parse_tool_call_args(raw) -> Result<Value, ArgsParseError> — fail-loud variant.
• parse_tool_call_args_lossy(raw) -> (Value, Option<String>) — migration bridge preserving current silent-{} behavior while populating ToolCall.arguments_parse_error for future gateway surfacing.
• probe_reasoning_field(json, &CANDIDATE_FIELDS) -> Option<&str> — ordered probe over candidate JSON reasoning-field names (handles reasoning_content vs reasoning vs reasoning_text divergence across OpenAI-compatible providers).
• ArgsParseError (thiserror-derived) with Display + std::error::Error impls.
• 12 inline unit tests covering valid/invalid inputs, primitives, probe ordering, non-string skip, empty-field slice, empty-string handling.

What this PR does NOT do

• No callers migrated. NearAI/Copilot/Gemini/Codex/AnthropicOAuth continue to do their own arg parsing — each gets its own follow-up PR per the recipe in the module doc-comment.
• No ToolCall contract change (Phase B).
• No NormalizingProvider decorator wiring (Phase C).
• No gateway behavior change. RC1 remains open until Phase C ships.

Code review

Multi-agent code review (8 reviewers in parallel) ran on this branch. 10 raw findings → 8 after dedup. Applied:

• ArgsParseError now derives thiserror::Error (Medium-90, also flagged by local-patterns + maintainability).
• 4 missing edge-case tests added (empty string in fail-loud variant, primitive JSON types, probe non-string skip, probe empty-fields slice).
• CLAUDE.md file-map row added for tool_args.rs.
• Cross-repo doc references stripped.

One finding deferred to Phase B planning: whether parse_tool_call_args_lossy should return (Value, Option<ArgsParseError>) instead of (Value, Option<String>) for consistency with the strict variant. Decision deferred so the lossy return shape can match the ToolCall.arguments_parse_error field shape Phase B chooses.

Test plan

• cargo fmt --check -p ironclaw_llm
• cargo clippy -p ironclaw_llm --all-features --tests — zero warnings
• cargo test -p ironclaw_llm tool_args — 12/12 pass

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request introduces a new module tool_args.rs to provide shared primitives for parsing provider tool-call responses, including JSON argument parsing and an ordered reasoning-field probe. It also updates CLAUDE.md and lib.rs to integrate and document this module. Feedback on the changes suggests optimizing probe_reasoning_field to return a borrowed &str instead of cloning the string, which avoids unnecessary memory allocations for potentially large reasoning content.

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.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d77aaf72b4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[Copilot]

Pull request overview

Adds a new internal (pub(crate)) tool_args module in ironclaw_llm that centralizes shared parsing primitives intended for future per-provider tool-call decoding migrations (RC3/M9 Phase A). This scaffolds strict vs. lossy JSON argument parsing plus an ordered reasoning-field probe, along with inline unit tests and crate/module documentation updates.

Changes:

• Introduces crates/ironclaw_llm/src/tool_args.rs with strict/bridge argument parsing helpers, a reasoning-field probe helper, and an ArgsParseError type.
• Exposes the module internally via pub(crate) mod tool_args; in crates/ironclaw_llm/src/lib.rs.
• Updates the crates/ironclaw_llm/CLAUDE.md file map to include the new module.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
crates/ironclaw_llm/src/tool_args.rs	New shared parsing primitives + unit tests for tool-call args and reasoning-field probing.
crates/ironclaw_llm/src/lib.rs	Registers the new internal tool_args module.
crates/ironclaw_llm/CLAUDE.md	Documents the new module in the crate file map.

---

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

[henrypark133]

Code Review (multi-agent)

Intent: Scaffold tool_args.rs shared parsing primitives for the RC3/M9 provider parsing framework (Phase A). No callers migrated.

Stats: 3 inline findings + 6 body-only notes (all prior-bot comments confirmed by my reviewers). From 14 raw across 8 reviewers; 0 reviewers failed.

Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, pattern-refactor

Prior bot comments — independently re-confirmed by my reviewers, NOT re-posted inline

All 6 prior comments from gemini-code-assist, chatgpt-codex-connector, and Copilot were re-flagged by one or more of my 8 reviewers. Leaving for the author to address; flagging here so the cross-reviewer signal is visible:

1. lib.rs:45 (chatgpt-codex) — pub(crate) mod tool_args will fail cargo clippy -D warnings with dead_code since no production callers exist yet. Also flagged by my conventions reviewer (Medium 80). Gate with #[cfg(test)] or #[allow(dead_code)] + tracked plan ref until Phase B caller lands.
2. tool_args.rs:46 (Copilot) — ArgsParseError.reason stores raw serde_json::Error::to_string() with no context prefix. My security and conventions reviewers raised the same concern (data-leakage / Map errors with context rule). Recommend format!("failed to parse tool-call arguments JSON: {e}") at the construction site AND/OR #[error("failed to parse tool-call arguments JSON: {{reason}}")] in the thiserror attribute. My local-patterns reviewer noted that all sibling errors in error.rs:24 use the context-prefix Display convention.
3. tool_args.rs:71 (Copilot) — parse_tool_call_args_lossy re-implements parsing instead of delegating to parse_tool_call_args. My conventions (Low 60) and maintainability (Low 75) reviewers raised the same DRY concern. Refactor to: check empty → return early; else delegate to strict + map error.
4. tool_args.rs:83 (Copilot) — probe_reasoning_field does not trim, so whitespace-only strings (e.g. " ") are returned as Some(" "). Diverges from ChatMessage::with_reasoning (provider.rs:173-176) which uses !r.trim().is_empty(). My bugs reviewer (Medium 80) and conventions reviewer agree. Fix: change !s.is_empty() → !s.trim().is_empty().
5. tool_args.rs:88 (gemini) — probe_reasoning_field clones a potentially-large reasoning String. My performance reviewer (Low 85) recommends returning Option<&'a str> borrowed from the input Value.
6. tool_args.rs:160 (Copilot) — Missing test for whitespace-only reasoning. My tests reviewer (Medium 85) agrees.

bugs

1. Medium parse_tool_call_args_lossy returns non-object Values with err=None — caller migration risk (crates/ironclaw_llm/src/tool_args.rs:60-70, confidence 65) — anchor: crates/ironclaw_llm/src/tool_args.rs:67
   When serde_json::from_str(raw) succeeds with a non-object (e.g. null, 42, [1,2,3], "hello"), the lossy variant returns (Value::Null|Number|Array|String, None) — no error, no fallback to {}. The strict variant's doc-comment explicitly defers shape decisions to callers, but the lossy variant is described as a 'migration helper preserving silent-{} behavior'. Existing call sites that us

tests

1. Medium No test pinning parse_tool_call_args_lossy non-object passthrough behavior (crates/ironclaw_llm/src/tool_args.rs:60-71, confidence 75) — anchor: crates/ironclaw_llm/src/tool_args.rs:67
   Existing tests exercise only the object case for the lossy variant. The current implementation passes through Value::Array / Value::Null / Value::Number / Value::String with err=None. A test documenting this contract — either pinning current passthrough behavior or asserting object-coercion if the bug-finding is acted on — prevents future shape-narrowing assumptions and guards the Phase B migratio

maintainability

1. Low parse_tool_call_args_lossy returns (Value, Option) — loses ArgsParseError type at boundary (crates/ironclaw_llm/src/tool_args.rs:60-60, confidence 65) — anchor: .claude/rules/types.md — prefer strong types over strings; ArgsParseError exists at crates/ironclaw_llm/src/tool_args.rs:34
   The lossy variant returns the error path as Option<String> rather than Option<ArgsParseError>. ArgsParseError exists in the same module (line 34) and already provides Display + std::error::Error. Returning a bare String means any caller wanting to log/propagate the error must work with untyped text, and forces downstream context-prefix decisions to live at every call site rather than in

[henrypark133]

Medium — parse_tool_call_args_lossy returns non-object Values with err=None — caller migration risk.

When serde_json::from_str(raw) succeeds with a non-object (e.g. null, 42, [1,2,3], "hello"), the lossy variant returns (Value::Null|Number|Array|String, None) — no error, no fallback to {}. The strict variant's doc-comment explicitly defers shape decisions to callers, but the lossy variant is described as a 'migration helper preserving silent-{} behavior'. Existing call sites that use unwrap_or(Value::Object(default)) always end up with an object; after migrating to parse_tool_call_args_lossy they can now receive non-object Values with no error signal, silently breaking the implicit ToolCall.arguments object invariant downstream tool-dispatch code relies on. Lossy isn't a like-for-like behavioural bridge for the unwrap_or pattern.

Fix: Either (a) add a shape guard inside lossy: on Ok(non-object), return (Value::Object(Map::new()), Some("expected JSON object, got <type>")); or (b) document the shape contract change in the function doc-comment and the Phase B migration recipe so per-provider PRs add their own shape guard. Pick a side before any caller is migrated.

match serde_json::from_str(raw) {
    Ok(Value::Object(map)) => (Value::Object(map), None),
    Ok(other) => (
        Value::Object(serde_json::Map::new()),
        Some(format!("expected JSON object, got {}", value_kind(&other))),
    ),
    Err(e) => (Value::Object(serde_json::Map::new()), Some(e.to_string())),
}

[henrypark133]

Deferred to Phase B planning. Pinned current behavior with the new parse_args_lossy_non_object_passthrough test in b37729e so the Phase B decision (coerce non-objects to empty object vs. keep passthrough) is made with the contract explicitly visible. Linked to the Option vs Option decision in the sibling thread — both shape the lossy variant's contract and belong together.

[henrypark133]

Low — parse_tool_call_args_lossy returns (Value, Option) — loses ArgsParseError type at boundary.

The lossy variant returns the error path as Option<String> rather than Option<ArgsParseError>. ArgsParseError exists in the same module (line 34) and already provides Display + std::error::Error. Returning a bare String means any caller wanting to log/propagate the error must work with untyped text, and forces downstream context-prefix decisions to live at every call site rather than in the type. .claude/rules/types.md: prefer strong types over strings. With the type, the Display format prefix fix (separately flagged in body notes) gets applied once and propagates everywhere.

Fix: Change the return type to (Value, Option<ArgsParseError>). Callers that only want the reason string can .map(|e| e.to_string()) or use Display. Note: this composes with the related Phase B/C type decision in the PR body — recommend resolving both together.

pub(crate) fn parse_tool_call_args_lossy(raw: &str) -> (Value, Option<ArgsParseError>) {
    if raw.is_empty() {
        return (
            Value::Object(serde_json::Map::new()),
            Some(ArgsParseError { reason: "empty arguments string".to_owned() }),
        );
    }
    match parse_tool_call_args(raw) {
        Ok(v) => (v, None),
        Err(e) => (Value::Object(serde_json::Map::new()), Some(e)),
    }
}

[henrypark133]

Deferred to Phase B planning per earlier session direction. Decision belongs alongside the shape of ToolCall.arguments_parse_error (Phase B contract change) — if that field lands as Option, the lossy return type Option is the direct pipe; if it lands as Option, the lossy return type should match. Leaving open so the Phase B planner resolves both together.

[Copilot]

Pull request overview

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

[henrypark133]

Re: multi-agent review — all items accounted for:

6 body-only notes = duplicate signal of the 6 prior bot comments (gemini/codex-connector/4× Copilot). All inline threads RESOLVED with replies referencing b37729e75:

• lib.rs:45 dead_code gate → #[allow(dead_code)] with tracked-plan-ref comment ✓
• tool_args.rs:46 ArgsParseError prefix → applied at construction inside parse_tool_call_args ✓
• tool_args.rs:71 lossy DRY → delegates to strict, only special-cases empty-string ✓
• tool_args.rs:83 whitespace trim → !s.trim().is_empty() matching ChatMessage::with_reasoning ✓
• tool_args.rs:88 borrowed return → Option<&str> (no String::clone on hot path) ✓
• tool_args.rs:160 whitespace-only test → probe_reasoning_field_skips_whitespace_only_values added ✓

3 inline findings:

• tests #1 non-object passthrough test → resolved, pinned current contract via parse_args_lossy_non_object_passthrough test ✓
• bugs #1 non-object passthrough behavior → deferred to Phase B planning (linked to the lossy variant's contract decision; making the call alongside ToolCall.arguments_parse_error shape)
• maintainability #1 Option<String> vs Option<ArgsParseError> → deferred to Phase B planning per earlier direction; sibling of bugs #1, both shape the lossy variant's contract

Plan doc at docs/...llm-normalizing-provider-framework.md updated to reflect Option<&str> signature so Phase B planner sees the shipped shape.

[henrypark133]

Code Review (multi-agent, re-review)

Intent: Phase A scaffold of tool_args.rs shared parsing primitives. Re-review after Fix #1-#7 round.

Stats: 4 net-new inline findings. 0 reviewers failed. Most prior comments verified addressed on current head.

Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, pattern-refactor

Prior-comment status

Addressed (verified on head 3cbc44f):

• ✅ Fix #1 (gemini): probe_reasoning_field now returns Option<&str> — no String clone on hot path.
• ✅ Fix #2 (chatgpt-codex): mod tool_args gated with #[allow(dead_code)] + scaffold comment.
• ✅ Fix #3 (Copilot): ArgsParseError.reason prefixed with failed to parse tool-call arguments JSON: context.
• ✅ Fix #4 (Copilot): parse_tool_call_args_lossy delegates to strict, only special-cases empty string.
• ✅ Fix #5 (Copilot): probe uses !s.trim().is_empty() matching ChatMessage::with_reasoning (provider.rs:173).
• ✅ Fix #6 (Copilot): probe_reasoning_field_skips_whitespace_only_values test added.
• ✅ Fix #7 (my prior): parse_args_lossy_non_object_passthrough test added pinning current contract.
• ✅ Copilot 2nd-round: PR-description vs impl mismatch on probe return type aligned to Option<&str>.

Deferred to Phase B (author replied with sound reasoning, NOT re-flagged):

• 🔁 lossy non-object → coerce vs passthrough — pinned with test, decision belongs alongside ToolCall.arguments_parse_error shape (Phase B contract).
• 🔁 (Value, Option<String>) vs (Value, Option<ArgsParseError>) — return shape decision deferred to Phase B alongside the same contract.

Re-considered but NOT flagged (covered by deferred items or author's design intent):

• probe_reasoning_field returns untrimmed &str despite trim-guard: author noted in Fix #5 reply that returning the raw value preserves provider intent. Reasonable design choice; not a bug.
• Drop ArgsParseError entirely in favor of Result<Value, String>: opposite direction of the deferred Phase B shape decision; collapses into that same item.

Net-new findings (inline)

security

1. Medium serde_json error position/token context propagated into surfaced reason string (crates/ironclaw_llm/src/tool_args.rs:43-46, confidence 65) — anchor: crates/ironclaw_llm/src/tool_args.rs:44
   Fix #3 added the 'failed to parse tool-call arguments JSON: ' prefix, but the raw {e} is still appended unsanitized. serde_json::Error::to_string() includes byte offset, line/column, and partial token context ('expected value at line 1 column 3'). The function's own doc-comment names ToolCall.arguments_parse_error as the gateway-surfacing destination in Phase B. That makes this a pre-plumbed dat

tests

1. Low Empty-string lossy error message not pinned by tests (crates/ironclaw_llm/src/tool_args.rs:61-65, confidence 75) — anchor: crates/ironclaw_llm/src/tool_args.rs:64
   parse_args_lossy_empty_string only asserts err.is_some() — the literal 'empty arguments string' text is not asserted. The parse-failure branch IS pinned to its 'failed to parse tool-call arguments JSON: ' prefix in parse_args_lossy_invalid_json_returns_empty_object_and_error. Without pinning the empty-string branch, a future rename or accidental routing through the format! path won't be caught.

conventions

1. Medium ArgsParseError defined in tool_args.rs, not error.rs (crates/ironclaw_llm/src/tool_args.rs:32-36, confidence 65) — anchor: CLAUDE.md (Code Style): "Use thiserror for error types in error.rs"
   CLAUDE.md (Code Style): 'Use thiserror for error types in error.rs'. The crate already has error.rs hosting LlmError + LlmConfigError as pub enums. ArgsParseError is a new thiserror-derived type placed in its owning module instead. Fragments the error hierarchy — future readers grep error.rs for all error variants and miss this one. Note this is orthogonal to the Phase-B-deferred shape decision (O

2. Low #[allow(dead_code)] on mod tool_args lacks tracked-issue arch-exempt link (crates/ironclaw_llm/src/lib.rs:44-48, confidence 55) — anchor: .claude/rules/architecture.md (annotation format: arch-exempt: <category>, <reason>, plan #NNNN)
   .claude/rules/architecture.md: 'listen to the language. When the compiler or clippy complains, the answer is almost never #[allow]' — and the documented annotation format is // arch-exempt: <category>, <reason>, plan #NNNN. The current annotation comment names the intent ('Phase A scaffolding — helpers awaiting per-provider migration (Phase B+)') but cites no plan number or tracking issue. No du

[henrypark133]

Medium — serde_json error position/token context propagated into surfaced reason string.

Fix #3 added the 'failed to parse tool-call arguments JSON: ' prefix, but the raw {e} is still appended unsanitized. serde_json::Error::to_string() includes byte offset, line/column, and partial token context ('expected value at line 1 column 3'). The function's own doc-comment names ToolCall.arguments_parse_error as the gateway-surfacing destination in Phase B. That makes this a pre-plumbed data-leakage path — once any provider calls parse_tool_call_args_lossy, the partial wire-input context lands in a caller-visible Option. Lock in sanitization at the API boundary now, before Phase B callers form expectations.

Fix: Reduce the embedded error to a position-only or generic shape before storing. e.g. derive line/column via e.line()/e.column() and discard e.to_string(), or strip the trailing 'at line L column C' tail in callers.

// Option A — keep only structured position
reason: format!(
    "failed to parse tool-call arguments JSON: invalid at line {} column {}",
    e.line(), e.column()
),

// Option B — strip serde-supplied context
reason: "failed to parse tool-call arguments JSON".to_string(),

[henrypark133]

Discussion needed — deferring.

The serde_json error position/token context (expected value at line 1 column 3) IS unsanitized. But for Phase A this is internal-only: pub(crate) helper, no callers, no channel crossing yet. Sanitization needs to happen at the gateway boundary when arguments_parse_error becomes model-visible (Phase B+ surfacing work), not at the provider parsing layer where it loses debug signal.

Two options to weigh:

1. Sanitize here — strip serde position/token context at the parsing layer. Loses debug signal in trace recordings; gateway gets nothing extra to log even when it would be safe.
2. Sanitize at the gateway — tool_args.rs keeps full debug detail; gateway redacts before emitting to channels (consistent with .claude/rules/error-handling.md 'Error Boundaries at the Channel Edge' rule, which is gateway-scoped).

I'd lean (2) since it matches the existing channel-edge sanitization pattern. But the call belongs to whoever plans the Phase B+ gateway surfacing — leaving open so it lands with that work.

[henrypark133]

Medium — ArgsParseError defined in tool_args.rs, not error.rs.

CLAUDE.md (Code Style): 'Use thiserror for error types in error.rs'. The crate already has error.rs hosting LlmError + LlmConfigError as pub enums. ArgsParseError is a new thiserror-derived type placed in its owning module instead. Fragments the error hierarchy — future readers grep error.rs for all error variants and miss this one. Note this is orthogonal to the Phase-B-deferred shape decision (Option vs Option); the location concern stands regardless of which return-shape decision lands.

Fix: Move ArgsParseError to crates/ironclaw_llm/src/error.rs and import via use crate::error::ArgsParseError in tool_args.rs.

// in error.rs
#[derive(Debug, thiserror::Error)]
#[error("{reason}")]
pub(crate) struct ArgsParseError {
    pub reason: String,
}

[henrypark133]

Discussion needed — leaving open.

CLAUDE.md says 'Use thiserror for error types in error.rs' — agreed on the thiserror part (applied in b37729e). On the location part, sibling precedent supports per-module pub(crate) error types: GithubCopilotAuthError lives in github_copilot_auth.rs:60, not error.rs. The rule's literal text is ambiguous between 'in error.rs, use thiserror' and 'all error types belong in error.rs and must use thiserror.'

ArgsParseError is pub(crate), used only by tool_args.rs callers. Moving to error.rs makes the central registry cleaner but creates an upstream dep from error.rs → tool_args semantics (or requires moving the type to be a pub enum variant of LlmError, which it isn't logically — it's a sub-step error not a top-level provider error).

Not resolving — flagging for project-wide convention clarification. If the convention is 'all errors in error.rs even for pub(crate) sub-step errors', happy to move it; if 'pub(crate) module-local errors stay with their module' (current precedent), leaving as-is.