Verify transcript parity across workflow spawn modes (#52)

[jameswnl]

Closes #52. Uses the merged lightspeed-core/lightspeed-cloud-agents#273 implementation. The dependency source is restored to branch = "main"; uv.lock resolves the merge commit 1788b17d84420ca07c8df423a460bcae5b369f66.

Summary

Verify canonical workflow transcripts through the real LocalWorkflowRunner and in-memory stores using a scripted mock LLM. Equivalent one-step and multi-step runs produce matching none/local events through the public transcript accessor. Shared assertions check event shape and data keys in none/local integration tests and none/ephemeral HTTP e2e tests.

The dependency update also changes chat history used by /query/direct and /query/direct/stream. Two-turn ChatWorkflowRunner regressions inspect actual provider requests for matching tool call/result IDs and preserved outputs. Large-argument coverage verifies that potentially truncated exchanges are omitted from replay while audit events remain intact. The mock rejects tool outputs whose call ID has no matching call in the request.

Failure tests cover none/local connection failures and output-schema parsing failures through both model-request and agent paths. Documentation explains aggregate/per-turn result-event differences, summing token usage, unknown cost, and bounded replay.

Ephemeral coverage remains contract coverage plus strengthened HTTP assertions; a captured live-sandbox golden transcript is a separate follow-up. No live ephemeral run was performed in this update.

Validation

• uv run make test-unit: 3,382 passed, 1 skipped; coverage 90.21%.
• Cloud-agents integration/unit suites: 140 passed, 1 skipped.
• Parity and mock-server suites: 25 passed against the installed dependency pin.
• Black, Ruff, docstyle and Pylint on changed test files pass.
• Full verify remains limited by Pylint findings in unchanged files, 11 Pyright errors in query_direct.py and question_validity/_capability.py, and one Mypy error in _capability.py. OpenAPI lint reports no files found; no API models changed.

The latest dependency also fixes mixed explicit/synthesized call IDs: explicit returns consume their pending call by ID, allowing out-of-order explicit results without leaving completed calls available to later ID-less turns. The 25 parity/mock tests and full stack unit suite were rerun successfully against this pin.

After the upstream merge, all 25 parity/mock tests and 3,382 stack unit tests passed again against the merged dependency.

[beesarmy]

Summary

Stack-side verification that the unified workflow path emits canonical transcript events across spawn modes. The core claim checks out: none/local now produce identical tool_call/tool_result/result skeletons end-to-end through the real LocalWorkflowRunner, failure runs persist a canonical error event, and per-mode gaps are documented. Approving — all findings below are non-blocking.

Verified independently:

• CI green (e2e-mock, unit-and-integration on 3.12/3.13); no other reviewer feedback pending.
• The pin 91d48ab matches the current head of upstream lightspeed-core/lightspeed-cloud-agents#273 (still OPEN), and uv.lock is consistent with pyproject.toml.
• Mock change is backward compatible: with tool_call=None, _select_body reduces exactly to the previous code path (same _response_text_for_request + _canned_response_body flow, including the json_schema structured-output branch).
• No src/ changes — verification-only on the stack side, as described. New mock helpers (_select_body, _canned_function_call_body, _input_has_function_call_output) each have direct unit tests, and both touched e2e tests extend real-HTTP assertions.

Findings (non-blocking):

1. [LOW] The commit message says "bump lightspeed-cloud-agents pin to rev 295a7a8" but the actual pin is 91d48ab (the re-bump after the evaluator round). Also, "lightspeed-cloud-agents#273" is ambiguous — there is no PR 273 in the jameswnl fork; the dependency is lightspeed-core/lightspeed-cloud-agents#273. Suggest fixing the rev in the commit message on merge and using the fully-qualified repo in the PR body.
2. [MED] The ephemeral leg of issue #52's acceptance criterion 1 ("the same workflow step yields the same transcript event types across none, local, and ephemeral") is covered at contract/shape level only: REFERENCE_EPHEMERAL_JSONL is hand-authored rather than captured from a real ephemeral run, and it contains a thinking event the scripted none/local runs never emit — so same-step event equality is asserted for none/local but not against a real ephemeral transcript. The PR body is transparent about the live-sandbox constraint, so this is follow-up material, not a merge blocker. Suggested follow-up: "Capture golden ephemeral transcript for parity tests — run the parity step once against a live sandbox, store the resulting event skeleton as the reference (replacing the hand-written REFERENCE_EPHEMERAL_JSONL), and assert none/local skeletons equal it."
3. [LOW] tests/integration/cloud_agents/test_workflow_transcript_parity.py imports from tests.e2e.cloud_agents.mock_llm_server — the first integration→e2e cross-suite import. Works fine, but if the mock gains more consumers, consider moving it to shared test helpers.
4. [LOW] _step_events reaches into the private runner._transcript_store (hence the module-level protected-access disable). The public get_step_transcripts() accessor — already used by test_get_step_transcripts_exposes_canonical_events — could serve all tests uniformly.
5. [LOW] Nits in the mock: _input_has_function_call_output assumes input is str-or-list (a truthy non-list would raise TypeError iterating); and the fixed call_id="call_mock_1" would be reused if the first turn were ever retried. Neither is reachable in these deterministic tests, but a defensive isinstance(input_value, list) guard would be cheap.

---

🤖 Reviewed by beesarmy via Claude Code

[jameswnl]

Review of PR #58: Verify transcript parity across workflow spawn modes (#52)

Checked at PR head ea3810e (base harness 70f7480) and the pinned cloud-agents 91d48ab (branch issue-transcript-parity, cloud-agents lightspeed-core#273). The 16 new and changed tests pass locally (test_workflow_transcript_parity.py + test_mock_llm_server.py, 35 s).

Summary: The stack-side tests are well built: real LocalWorkflowRunner, in-memory stores, a scripted tool call, and a RED check against the old pin. But the new cloud-agents pin brings in a regression on /query/direct follow-up turns, and the "ephemeral" half of the parity claim is only lightly tested.

---

Blocking

1. The pin breaks /query/direct follow-up turns after a tool call

• At 91d48ab, ChatWorkflowRunner._save_turn builds tool_call / tool_result conversation messages from the new canonical events and leaves out tool_call_id, relying on _build_message_history's fallback call_<tool>_<n>.

• That fallback adds 1 to the counter on both the call and the result (direct.py:294 and :313), so the pair never matches. Repro against the installed package:

  ToolCallPart   call_kubectl_get_0
  ToolReturnPart call_kubectl_get_1
  

• Providers reject a tool result whose id matches no tool call (OpenAI function_call_output.call_id, Anthropic tool_result.tool_use_id). So on /query/direct and /query/direct/stream, the next turn after any turn that ran an MCP tool will fail.

• This is new. Before this pin, DirectExecutor only emitted a flat agent.run / agent.stream entry, so no tool messages were saved and history simply had none.

• Second problem in the same code: tool_result events carry no tool name, so the runner carries the name forward from the last tool_call. With parallel tool calls (call A, call B, result A, result B), both results get name B.

Proposed: fix in cloud-agents lightspeed-core#273 before repinning. Pair results with calls in order (a FIFO of pending (name, id) from the tool_call messages), and only advance the counter on tool_call. On the stack side, add a two-turn /query/direct test with the scripted tool, and make the mock strict: return 400 when a function_call_output.call_id has no matching function_call in the same input. Today the mock accepts any id, which is why nothing caught this.

---

Should fix

2. Pin details

• The commit message says rev 295a7a8; pyproject.toml and uv.lock pin 91d48ab (one more commit on top). Fix the message.
• The pin is a commit on an unmerged fork branch. If that branch is rebased or deleted, the commit can be garbage-collected and uv sync --frozen breaks. Merge MGMT-21204: add categories to feedback API lightspeed-core/lightspeed-stack#273 first, or push a tag for 91d48ab.

3. Ephemeral parity is checked only lightly

• TestEphemeralContract runs cloud-agents' normalize_transcript_events on a hand-copied EventLogger fixture. That tests the normalizer, not the sandbox, and the copy will drift silently.
• The ephemeral e2e test checks only event keys and type names, not data keys.
• Proposed: add one helper that checks data keys per type (tool_call: {name, input}, tool_result: {output}, thinking: {text}, result: {text, cost_usd, input_tokens, output_tokens}, error: {message}) and use it in the none, local and ephemeral tests.

4. Failed-run test covers none only

subprocess_child.py also changed to emit an error event. Parametrize test_failed_run_emits_error_event over none and local.

5. Docs: say how to read result events

The doc calls the per-mode differences "not shape differences", but the number of result events differs (one per run for none/local, one per turn for ephemeral). Say what consumers must do: sum tokens over all result events, treat cost_usd: null as unknown (not 0), and take the final answer from the last result event. Otherwise someone reads only the last event and undercounts ephemeral usage.

6. New data surface (link to #51)

none/local transcripts now store tool inputs and outputs (up to 2,000 characters each) and the full error text, in the transcript store and in GET /v1/workflows/{id}/transcripts. /query/direct turn records now also store tool args and results in conversation messages. Before, none stored only model and token metadata. This matches ephemeral, so it isn't a new kind of exposure, but it is a new surface for none/local and /query/direct. Not blocking here; add "chat turn messages (tool args/results)" to the #51 Phase 4 canary list.

---

Nits

• The local tests work only because the subprocess child inherits the full os.environ (OPENAI_BASE_URL, CLOUD_AGENTS_TOOLS_MODULE) and the repo-root working directory. That is Design stack-side configuration and secret resolution for unified agent workflows #51 gap G8; when OLS-1946: Fix contributing.md for uv lightspeed-core/lightspeed-stack#269 trims the child env these tests will fail. Add a comment so that failure is read correctly, and consider setting PYTHONPATH explicitly instead of relying on cwd.
• _ResponsesHandler.response_text: str lost its default. Fine while the handler is built through type(...), but a direct subclass now fails at runtime. Keep = DEFAULT_RESPONSE_TEXT.
• The mock's _input_has_function_call_output scans the whole input. In a multi-turn chat the earlier function_call_output stays in history, so the mock never scripts a second tool call. Fine for now; say so in the docstring.
• @pytest.mark.asyncio is redundant with asyncio_mode = "auto".
• Importing parity_tools registers parity_probe in the global tool registry for the rest of the test session.
• cloud-agents transcript_events_from_messages docstring says result.all_messages(); callers correctly pass new_messages(). Fix the docstring.

Verified

• ffdc893 (the old lock) is an ancestor of 91d48ab; the only changes over cloud-agents main are the two parity commits.
• The stack doesn't parse transcript events itself, so no stack code depends on the old flat shape.
• The PR's test counts match (9 parity tests: 7 runner + 2 contract).
• Note for the Design stack-side configuration and secret resolution for unified agent workflows #51 plan: its line references are against ffdc893; direct.py and chat/runner.py line numbers shift with this pin.

[beesarmy]

Reviewed current head 6dd38f2 and cloud-agents pin 91d48ab. The focused parity and mock-server suites pass: 16 tests, using the existing stack virtual environment with PYTHONPATH explicitly selecting the exact pinned cloud-agents source and this PR checkout. No live ephemeral sandbox or full suite was run.

The stack-side one-step/multi-step verification is useful, but this dependency bump also changes the ChatWorkflowRunner used by /query/direct and /query/direct/stream. The pin still contains the blocking tool-history regression described in the existing owner comment. This review supersedes beesarmy's earlier approval of ea3810e: passing workflow parity tests do not make the pin safe for multi-turn chat.

I independently reviewed cloud-agents lightspeed-core#273 and posted three reproduced findings: mismatched call/result IDs and parallel-call names (P1), truncated arguments persisted as invalid JSON for replay (P2), and local output-schema failures emitting only result rather than error events (P2). See lightspeed-core/lightspeed-cloud-agents#273 (review). Please repin pyproject.toml and uv.lock after those fixes.

For regression coverage, add a two-turn chat/direct-query case that performs a tool call and replays the saved turn, including a large argument. The new mock currently returns HTTP 200 even when function_call_output.call_id does not match the function_call in input; I reproduced that behavior, so it would mask the provider rejection unless strengthened. The current failure-parity test exercises only spawn:none connection failures; a spawn:local output-schema parsing failure would expose the missing error event.

The hand-authored ephemeral reference remains contract coverage rather than a real three-mode comparison, as already documented in the PR and earlier review. I am not treating that separately tracked follow-up as an additional blocker.

[beesarmy]

[P1] Repin after fixing the tool-history regression in cloud-agents

This revision changes ChatWorkflowRunner._save_turn to persist tool messages without IDs, but its _build_message_history increments the fallback counter for both a call and its return. Replaying a single parity_probe exchange produces call_parity_probe_0 for the call and call_parity_probe_1 for the return. The stack's workflow/query_executor.py uses this ChatWorkflowRunner for /query/direct and /query/direct/stream, so the next chat turn after tool use sends an unmatched tool output to the provider and can fail. Multiple calls also inherit the last call's name for every result. This is a runtime regression introduced by the dependency bump, despite the workflow-only parity tests passing. Fix the pairing upstream and update both this pin and uv.lock before merging; add save/replay or two-turn direct-query coverage with matching-ID validation. Upstream reproduction and requested fixes: lightspeed-core/lightspeed-cloud-agents#273 (review).

[beesarmy]

Re-reviewed b946979 and its cloud-agents pin b5a1902. The earlier stack review requests have been addressed: the pin and lock agree; the mock rejects unmatched tool-output IDs; two-turn chat regressions inspect actual provider requests and preserve the real small-argument output; large-argument exchanges are excluded from replay; both spawn modes and both parsing paths have failure assertions; and shared canonical data-key checks now cover the strengthened HTTP tests. No additional stack-specific defect found in these changes.

Validation: 25 parity/mock-server tests passed against these exact two source checkouts using the existing stack virtual environment with explicit PYTHONPATH. The focused cloud-agents suite also passed 194 tests. No live ephemeral sandbox or full suite was run. The live golden-transcript follow-up remains documented.

I found one remaining dependency issue while auditing the updated pairing implementation: explicit-ID results leave stale entries in pending_calls, so a later ID-less result reuses a completed call's ID when replaying mixed history. I reproduced this across two turns. The pinned revision includes that bug, so please update the pin/lock once the small upstream correction lands. Full finding: lightspeed-core/lightspeed-cloud-agents#273 (review).

[beesarmy]

[P2] Repin after the remaining mixed-history pairing fix

The updated pin fixes the original canonical-only replay failures, but _build_message_history still keeps calls in its pending queue after an explicitly identified return. Replaying an explicit-ID prior turn followed by a new ID-less tool turn produces old_tool call/return = old-id, new_tool call = call_new_tool_1, new_tool return = old-id. This affects the same ChatWorkflowRunner used by the direct-query bridge whenever it loads mixed history. The new tests exercise entirely ID-less saved turns, so all 25 focused stack tests pass without exposing this case. Please bump this pin and uv.lock after the upstream queue fix; see lightspeed-core/lightspeed-cloud-agents#273 (review).

[beesarmy]

Coverage follow-up for head b946979, tested with cloud-agents source pin b5a1902:

• Broader cloud-agents integration/unit suites plus mock-server tests: 149 passed, 1 skipped.
• mock_llm_server.py: 96% line / 93% branch coverage; all 45/45 added executable lines and all branch arcs starting on added lines were exercised.
• workflow_e2e_helpers.py: all 8/8 added executable lines in the canonical-event helper were exercised. Whole-file coverage is lower (56% line / 25% branch) because existing live HTTP/database helpers are outside these suites; it does not indicate an uncovered new validator.

The small-argument and large-argument two-turn chat cases, orphan-ID mock rejection, both spawn modes' connection failures, and all four spawn/path combinations for schema parsing failures are exercised. Shared canonical data-key assertions run through none/local integration tests; the real ephemeral HTTP assertion still requires live-sandbox execution and was not run here.

The remaining gap is mixed explicit/synthesized tool-ID history, identified upstream. All current two-turn regression cases begin with new ID-less persistence, so they do not enter the broken explicit-ID return branch. The full cloud-agents unit suite also leaves that branch uncovered. Please include that case with the upstream fix before repinning. Coverage details: lightspeed-core/lightspeed-cloud-agents#273 (review).

[beesarmy]

Approved after the requested 30-minute follow-up review of head 71de7f0. pyproject.toml and uv.lock consistently pin cloud-agents 1194ff0, which fixes the remaining mixed-history queue bug. I verified the matching-ID correction, its mixed-ID/out-of-order regressions, and the original reproduction upstream; all blocking findings from my earlier reviews are resolved. No new stack-specific defect found.

Validation and coverage:

• 149 cloud-agents integration/unit and mock-server tests passed locally, with 1 skip, using this exact stack checkout and dependency source.
• Mock server: 96% line / 93% branch coverage; all 45 added executable lines and all branch arcs starting on added lines are covered.
• Canonical-event helper: all 8 added executable lines are covered; overall helper coverage is lower because existing live HTTP/database utilities are outside the local suites.
• Upstream focused/integration tests passed 206 cases with branch coverage; the previously missing explicit-ID handling and new matching/removal logic are exercised.
• Current-head CI is green for unit/integration on Python 3.12 and 3.13 and for e2e-mock.

The live ephemeral golden-transcript comparison remains the documented follow-up; no live sandbox was run locally. The earlier non-blocking upstream coverage suggestions remain follow-up work rather than merge blockers.