A controller's response shape depends on whether its handler happened to log

[M3gA-Mind]

Summary

A controller's response shape depends on whether its handler happened to log

Found during the e2e coverage wave and independently verified by a second engineer who did not
write the original report — verification was cross-assigned so nobody checked their own finding.

• Namespace / method: cross-cutting — RpcOutcome::into_cli_compatible_json,
  the shared return path for most controllers. Observed on
  openhuman.javascript_list_tools / openhuman.javascript_execute_tool vs
  openhuman.skills_read_resource / openhuman.skills_recent_runs.
• Where: src/rpc/mod.rs:54-62
• Verified at: openhuman main @ d1ed68f9b

Problem

Expected

one namespace, one response shape. A caller writes one parser.

Actual

pub fn into_cli_compatible_json(self) -> Result<Value, String> {
    let RpcOutcome { value, logs } = self;
    let value = serde_json::to_value(value)?;
    if logs.is_empty() {
        Ok(value)                                  // bare
    } else {
        Ok(json!({ "result": value, "logs": logs })) // wrapped
    }
}

The envelope is decided by the log vector, not by the method. So:

method	logs	response
javascript_list_tools	always 1 line	{result: {tools: […]}, logs: […]}
skills_read_resource	Vec::new()	{workflow_id, content, bytes, …} bare
skills_recent_runs	Vec::new()	{runs: […]} bare

Both spellings live in the same skills/javascript family and neither is
documented in the controller catalog — the ControllerSchema outputs describe
the inner value in both cases, so the schema is silently right for one and
silently wrong for the other.

The failure mode is nasty because it is conditional: a handler that gains a
tracing-adjacent log line later changes its own wire shape without any
schema change, and a caller that worked yesterday reads undefined today.
Nothing in CI would catch it.

Steps to reproduce

tests/raw_coverage/skill_runtime_e2e.rs →
javascript_bridge_lists_and_executes_a_real_tool, which asserts the wrapped
form explicitly before unwrapping, so the day this is normalised the suite
fails loudly. Found the hard way: the first run of that case died on
no 'tools' array: {"logs":["javascript.list_tools: count=227"],"result":{"tools":[…]}}.

Independent verification (CONFIRMED — and the blast radius is materially larger than the report says.)

Cited location still accurate: yes, exactly. src/rpc/mod.rs:54-62, quoted
verbatim and unchanged.

Repro run: cargo test --test raw_coverage_all --features "$(bash scripts/ci/product-features.sh)" -- W5VERIFY_
plus grep -rln "'result' in " app/src/ | xargs grep -ln logs

Observed

test W5VERIFY_skill_runtime_e2e::javascript_bridge_lists_and_executes_a_real_tool ... ok

Per-method construction confirmed at source:

// src/openhuman/runtime/node/rpc.rs:26-33  → non-empty log → WRAPPED
let log = vec![format!("javascript.list_tools: count={}", …)];
RpcOutcome::new(payload, log).into_cli_compatible_json()

// src/openhuman/skills/schemas/handlers.rs:147-150 → Vec::new() → BARE
to_json(RpcOutcome::new(WorkflowsRecentRunsResult { runs }, Vec::new()))
// src/openhuman/skills/schemas/handlers.rs:203-211 → Vec::new() → BARE

Diagnosis assessment: correct, and I hit this independently while writing the W4
suites — billing_get_balance (RpcOutcome::single_log) comes back wrapped while
cost_get_dashboard (RpcOutcome::new(_, Vec::new())) comes back bare, from two
handlers a caller would reasonably treat as one family. My w4_shared::peel() helper
exists solely to absorb this, which is corroboration from a second worker who did not
know of this report.

Is the expected behaviour correct: yes — "one namespace, one response shape" is
the right expectation, and the report is right that fixing it is a breaking change
needing a migration rather than a quiet normalisation.

Solution

as the report says — make the envelope
unconditional, or bare with logs on a side channel. Whichever is chosen, the 13 files
below are the migration surface. Interim mitigation with no breaking change: have the
controller catalog record which shape each controller returns, since that is
derivable from whether the handler passes a non-empty log vector.

Blast radius:

Acceptance criteria

• Repro gone — the reproduction above no longer produces the reported behaviour.
• Regression safety — coverage added or updated so this cannot come back silently.
• Diff coverage ≥ 80% — the fix PR meets the changed-lines coverage gate.
• Blast radius checked — every consumer named above still behaves correctly.

Related

Filed from the e2e coverage wave (#6053, #6054, #6055, #6058, #6059, #6060), which covered 244
previously untested RPC controllers and surfaced 33 defects along the way.

[tinysweeper]

A controller's response shape depends on whether its handler logs, causing inconsistent wire formats (bare vs wrapped with logs) across controllers in the same family. This can silently break callers when a handler gains a log line, with no schema change or CI catch.

Labelled priority: p2.

[M3gA-Mind]

I picked this up to fix and stopped short of normalising the envelope, because that is still a maintainer's call. What I can do is remove the reasons it is hard to decide. Four findings, three of which are not in the issue.

1. The envelope is implemented twice, and the issue names one

src/rpc/mod.rs:54 is cited. There is a second, independent copy at src/core/types.rs:67:

pub fn invocation_to_rpc_json(inv: InvocationResult) -> serde_json::Value {
    if inv.logs.is_empty() { inv.value }
    else { json!({ "result": inv.value, "logs": inv.logs }) }
}

Same rule, written out separately, with nothing linking them. It is live: core/dispatch.rs:64 uses it for the dynamic-dispatch path (internal / legacy methods). A fix applied only to the cited location would have left that path on the old behaviour — and the divergence would have been invisible until a caller hit the wrong path.

2. The migration surface is two functions, not ~450 call sites

I previously estimated this as ~106 bare + ~320 single_log construction sites and said it was an order of magnitude beyond the issue's "13 files". That framing was wrong and I want to correct it. The construction sites do not need editing — the shape is decided entirely inside the two functions above. Normalising is a change to two function bodies. The breadth is in the behaviour, not the diff.

3. Nothing consumes log content

Every .logs reference in app/src is either an envelope discriminator or unrelated state:

site	what it does
utils/tauriCommands/common.ts:81	Array.isArray(logs) — type guard
lib/composio/composioApi.ts:45	type guard
services/api/runtimeToolsApi.ts:33	guard, then return record.result
services/api/channelConnectionsApi.ts:53	guard, then return record.result
services/api/flowsApi.ts:340	guard, then return record.result
store/accountsSlice.ts:54,63,124	unrelated — per-account log state

So the wrapped envelope carries a payload no frontend code reads. It is a discriminator, nothing more.

4. The peelers are defensive; there is no central peel

goalsApi.ts:47, runtimeToolsApi.ts:33, channelConnectionsApi.ts:53 and flowsApi.ts:340 all have the same shape — unwrap if {result, logs}, otherwise pass the payload through. They tolerate either shape.

But coreRpcClient.ts:861-866 peels only the outer JSON-RPC result, not the inner envelope. So there is no single place that normalises this, and the majority of API modules have no peel helper at all — they read the bare shape directly.

What that implies for the two options

• Always wrapped — breaks every consumer that reads the bare shape without a defensive peel, which is most of them. Highest risk.
• Always bare (drop the envelope) — the four defensive peelers fall through correctly, the non-defensive readers already expect bare, and per finding 3 nothing loses information it was using. On this evidence it is the low-risk direction, with one caveat I could not fully settle: into_cli_compatible_json is named for CLI compatibility, and a CLI user currently sees the logs inline in the JSON. Dropping them is a UX change for that surface even though no programmatic consumer reads them.

I have deliberately not picked. That is the ruling this issue needs, and it should be one line in apply_log_envelope.

What I did ship

PR to follow: it makes the rule exist once — both paths delegate to a new rpc::apply_log_envelope, with the current behaviour preserved byte-for-byte — plus a test asserting the two paths agree for the same (value, logs) pair. The test asserts agreement, not a shape, precisely so it does not enshrine the defect as intended.

That is behaviour-neutral and leaves the decision open, but when the ruling lands it is a one-line change in one body instead of two, and the guard makes re-forking them a failing test rather than a silent regression.