Repository navigation
chore: promote staging to staging-promote/8753c482-23098316440 (2026-03-15 00:13 UTC) - #1188
Conversation
* Make no-panics check test-aware * Handle proc-macro test attrs in no-panics check * Pin Python for no-panics CI job
* Fix schema-guided tool parameter coercion * Fix CI checks for coercion regression tests * Finish panic-scan annotations * Avoid redundant worker param preparation * Keep panic-scan annotations rustfmt-stable * Handle nullable WASM schema review feedback * Address param coercion review notes
github-actions
Bot
added
scope: agent
Code reviewFound 2 issues: |
|
[HIGH:78] Unbounded JSON parsing in parameter coercion The new prepare_tool_params() calls serde_json::from_str() on LLM params without timeout protection. Happens in execute_tool_with_safety() BEFORE the execution timeout. If LLM sends malicious deeply nested JSON, could cause unbounded memory allocation. Recommendation: Move coercion inside timeout guard or add size validation. |
|
[MEDIUM:72] Test-aware panic check may miss panics The bash/awk no-panics check determines test scope per hunk based on hunk header context. However, hunk headers show function names, not 'mod tests' scope. Production code after test hunks could be misclassified as test code. Recommendation: Python check (scripts/check_no_panics.py) is more accurate via AST parsing. |
|
Review summary: Found 2 issues. The HIGH severity issue (unbounded JSON parsing in coercion) should be addressed before merging. The MEDIUM severity issue (panic check accuracy) is a trade-off in the new bash implementation. Overall, the refactoring to centralize parameter coercion in coercion.rs is a good architectural improvement. |
|
[CRITICAL:85] Unbounded recursion in coerce_value() causes stack overflow The recursive JSON coercion function (src/tools/coercion.rs:15-69) has no depth limits. Deeply nested JSON (10k+ levels) will overflow the stack. This is a production blocker when processing external/untrusted tool parameters. Recommendation: Add max recursion depth check or use iterative approach with explicit stack. |
|
[HIGH:80] Excessive cloning in recursive coercion creates O(n²) memory usage Multiple .clone() calls on JSON values in recursive path (lines 21,25,30,34,41,52,65,68). Large/nested structures cause cascading allocations. Example: 1MB input creates multiple MB during array/object coercion. Recommendation: Use references where possible or implement iterator-based approach to avoid cloning. |
|
[HIGH:75] Unbounded object property iteration enables DOS Lines 54-63 iterate all object properties without size limits. Combined with recursive cloning, a 10k-property object creates multiplicative allocations. Attacker could send {a:1MB, b:1MB...} with 10k properties to exhaust memory. Recommendation: Add per-object property count limits or size limits to coercion function. |
REVIEW COMPLETE - SUMMARYFound 6 issues total: CRITICAL (2):
HIGH (2):
MEDIUM (2):
RECOMMENDATION: Do NOT merge until CRITICAL issues are resolved. The recursion, cloning, and unbounded resource consumption create production outage risks. |
|
[MEDIUM:65] Missing error context in coercion per CLAUDE.md src/tools/coercion.rs:97 uses .ok()? for JSON parsing without error context. Per CLAUDE.md code style: 'Map errors with context: .map_err(|e| SomeError::Variant { reason: e.to_string() })?' Currently silently drops parse failures. When coercion fails, tool execution proceeds with wrong types and errors surface later with poor context about the original stringified value. Recommendation: Map parse errors with context about what value failed to parse. |
|
[CRITICAL:92] TOCTOU race condition in tool approval with hook modification The approval check uses normalized params (line 486-489), but BeforeToolCall hook (line 521-564) can modify params AFTER approval is checked. The hook-modified params are re-normalized (line 552) but approval is NOT re-checked. Attack: Hook transforms parameters to bypass approval. Example: {'safe': 'false'} (string, requires approval) → hook changes to {'safe': 'true'} but approval was already decided on the string value. Mitigation: Must re-check tool.requires_approval() after hook returns modified params. |
|
[CRITICAL:92] Unbounded recursion + JSON parsing creates DOS vector (DUPLICATE FINDING) This confirms the earlier finding. Attacker can craft: {"param": "[[[[...]]]]]"} with 10k nesting levels. Sequence: Line 97 parses JSON (serde_json allows arbitrary depth) → Line 105 calls coerce_value() → Line 40 recurses on array items. Combined recursion exhausts stack and crashes process. CRITICAL: This is exploitable by any LLM-provided tool parameter. |
|
[HIGH:72] Hook modification can bypass parameter validation When hook returns modified params (line 548), code re-normalizes (line 552) but validation still runs on normalized values (line 574-590). Risk: Hook could return injection payload as string. This normalizes to string, passes validation, and reaches tool. While tools should validate inputs, this violates defense-in-depth. Mitigation: Re-validate after hook modification. |
SECURITY ALERT - MERGE BLOCKEDAll 4 review agents have identified CRITICAL security issues: CRITICAL (2):
HIGH (2):
Action Required:
These are not design issues but concrete exploitable vulnerabilities. DO NOT MERGE until fixed. |
b8ddbea
into
staging-promote/e805ec61-23059634819
…3099429381 chore: promote staging to staging-promote/8753c482-23098316440 (2026-03-15 00:13 UTC)
…3099429381 chore: promote staging to staging-promote/f8d81a89-23098316440 (2026-03-15 00:13 UTC)
1 participant
Auto-promotion from staging CI
Batch range:
8753c482334b72dd97773b1d7c0e9ffbcba4c77f..c79754df2888ac7e2704d6cf4686b111eceee959Promotion branch:
staging-promote/c79754df-23099429381Base:
staging-promote/8753c482-23098316440Triggered by: Staging CI batch at 2026-03-15 00:13 UTC
Commits in this batch (2):
Current commits in this promotion (2)
Current base:
staging-promote/8753c482-23098316440Current head:
staging-promote/c79754df-23099429381Current range:
origin/staging-promote/8753c482-23098316440..origin/staging-promote/c79754df-23099429381Auto-updated by staging promotion metadata workflow
Waiting for gates:
Auto-created by staging-ci workflow