fix: gateway_execute args schema so callers can actually pass arguments - #107
Conversation
serde_json::Value has no fixed JSON type, so schemars emitted a typeless
schema for GatewayExecuteRequest.args — callers had no signal to send a
nested object rather than a stringified one, and mcp_stdio's call() then
rejected the stringified value. Dogfooding gateway_execute against a real
downstream server reproduced this on every non-empty args call. Switching
to Option<Map<String, Value>> makes schemars emit {"type": ["object",
"null"]}, a real hint, while the runtime behavior (Object or Null, nothing
else) is unchanged.
📝 WalkthroughWalkthroughThis PR adds a “Memory on-demand” line to the session-start message and updates ChangesMemory nudge and typed gateway args
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/mcp_server.rs (1)
73-80: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a schema regression test for
args.This PR’s key contract is the generated schema hint, but the updated tests only exercise runtime construction. Please lock the
argsschema to object/null so future type changes don’t silently reintroduce the typelessValueschema.Example regression check
+ #[test] + fn gateway_execute_args_schema_is_object_or_null() { + let schema = schemars::schema_for!(GatewayExecuteRequest); + let schema_json = serde_json::to_value(&schema).unwrap(); + let args_schema = schema_json + .get("properties") + .and_then(|properties| properties.get("args")) + .expect("args schema should be present"); + + let rendered = args_schema.to_string(); + assert!(rendered.contains("\"object\""), "{rendered}"); + assert!(rendered.contains("\"null\""), "{rendered}"); + }🤖 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 `@src/mcp_server.rs` around lines 73 - 80, Add a schema regression test for the `args` field in `McpServer` so the generated schemars output is locked to an object-or-null shape. Extend the existing test coverage around `args: Option<serde_json::Map<String, serde_json::Value>>` to assert the schema includes an object hint rather than a typeless `Value` schema, and verify the `McpServer`/`args` contract directly so future type changes are caught.
🤖 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.
Nitpick comments:
In `@src/mcp_server.rs`:
- Around line 73-80: Add a schema regression test for the `args` field in
`McpServer` so the generated schemars output is locked to an object-or-null
shape. Extend the existing test coverage around `args:
Option<serde_json::Map<String, serde_json::Value>>` to assert the schema
includes an object hint rather than a typeless `Value` schema, and verify the
`McpServer`/`args` contract directly so future type changes are caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9503b312-1bd2-41be-a6bf-5121f63fafb4
📒 Files selected for processing (2)
src/hook.rssrc/mcp_server.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/mcp_server.rs (1)
738-741: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the schema type structurally.
to_string().contains(...)can pass if"object"/"null"appear outside the actualtypeconstraint. Since this test locks the publicgateway_execute.argscontract, inspect thetypefield directly.Proposed test tightening
- let rendered = args_schema.to_string(); - assert!(rendered.contains("\"object\""), "{rendered}"); - assert!(rendered.contains("\"null\""), "{rendered}"); + let mut types: Vec<_> = args_schema + .get("type") + .and_then(serde_json::Value::as_array) + .expect("args schema type array") + .iter() + .filter_map(serde_json::Value::as_str) + .collect(); + types.sort_unstable(); + assert_eq!(types, vec!["null", "object"], "{args_schema}");🤖 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 `@src/mcp_server.rs` around lines 738 - 741, The test in the schema assertion block is too loose because it checks the rendered JSON string for "object" and "null" instead of validating the actual type structure. Update the assertions around the args schema in the relevant test to inspect the schema JSON directly via the args_schema value, specifically checking the type field (or equivalent structural representation) for the expected object/null shape. Keep the change localized to the test that verifies the gateway_execute.args contract.
🤖 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.
Nitpick comments:
In `@src/mcp_server.rs`:
- Around line 738-741: The test in the schema assertion block is too loose
because it checks the rendered JSON string for "object" and "null" instead of
validating the actual type structure. Update the assertions around the args
schema in the relevant test to inspect the schema JSON directly via the
args_schema value, specifically checking the type field (or equivalent
structural representation) for the expected object/null shape. Keep the change
localized to the test that verifies the gateway_execute.args contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 75c60914-0468-4da5-8b92-6a0c73c7a1cd
📒 Files selected for processing (1)
src/mcp_server.rs
…task-level test (#480) * fix(daemon): make binary-staleness watchdog failures observable, add task-level test Item #107: a live daemon kept running for 19+ minutes after its on-disk binary was replaced, missing ~19 BINARY_STALENESS_CHECK_INTERVAL checks, with no log line ever indicating whether the watchdog was armed. The comparison logic in BinarySnapshot::is_stale() already had unit coverage and reads correctly; what was missing was any signal, at the live-daemon level, of whether the watchdog task ever armed in the first place -- capture() failing at startup and the watchdog silently no-op'ing was indistinguishable from "armed, nothing stale yet". Split the polling loop out of spawn_binary_staleness_watchdog into daemon::wait_for_stale so it has its own test coverage independent of is_stale()'s unit tests -- exercised here with a simulated tokio clock covering multiple ticks and a real on-disk file swap. Also log explicitly on both the armed and disabled-at-startup paths so a silent failure to arm shows up in daemon.log instead of looking identical to normal operation. Agentflare-Agent: claude-code Agentflare-Branch: task/107-binary-staleness-watchdog-doesn-t-self-r Agentflare-Item: 107 * chore: regenerate workspace-hack for tokio test-util feature The new #[tokio::test(start_paused = true)] regression test in src/daemon.rs pulled in tokio's test-util feature, changing the workspace-wide feature union cargo hakari computes. Regenerate agentflare-workspace-hack/Cargo.toml to match, which is what CI's own cargo hakari generate --diff check enforces. Agentflare-Agent: claude-code Agentflare-Branch: task/107-binary-staleness-watchdog-doesn-t-self-r Agentflare-Item: 107 --------- Co-authored-by: shiva <shiva@gosysinfo.tech>
Summary
GatewayExecuteRequest.argswas typed as bareserde_json::Value, which schemars renders with notypeconstraint — callers had no signal to send a nested JSON object, sogateway_executecould never actually be invoked with arguments (confirmed live via dogfooding against theengrambackend).Option<serde_json::Map<String, serde_json::Value>>, which schemars renders as{"type": ["object", "null"]}. Runtime semantics are unchanged (accept Object or Null, reject anything else).Test plan
cargo test— 206/206 passinggateway_search→gateway_execute(engram, memory_context, {...})succeeds end-to-end post-fixSummary by CodeRabbit
New Features
Bug Fixes